Code review: Dirty cache read/fetch

[riking]

Objective:

Return the *Channel object, containing metadata about the Channel, corresponding to the given ID. The cache must be updated from a network service every 24 hours, but it’s okay to return stale data.

Unusual design decisions:

If we get an explicit “that ID does not exist” from the network service, a .NotExist = true object with a valid CacheTS is stored in the map.
If the ID is missing from the map, we do a synchronous network fetch (!)

The function feels really messy. Any tips on how to clean it up? I need to repeat the function for other types, as well.

type DataCache struct {
	// ...
	channelLock sync.RWMutex
	channelInfo map[ChannelID]*Channel
	// ...
}

func (c *DataCache) ChannelInfo(channel ChannelID) (*Channel) {
	c.channelLock.RLock()
	defer c.channelLock.RUnlock()

	ch, ok := c.channelInfo[channel]
	needsUpdate := 0
	if !ok {
		needsUpdate = 2
	} else if ch.CacheTS.Before(time.Now().Add(-24 * time.Hour)) {
		needsUpdate = 1
	}
	if needsUpdate == 2 {
		// new stack frame to protect against panic causing double-unlock
		func() {
			c.channelLock.RUnlock()
			defer c.channelLock.RLock()
			_, _ = c.updateChannelInfo(channel)
		}()
		ch, ok = c.channelInfo[channel]
		if !ok {
			return nil
		}
	} else if needsUpdate == 1 {
		go c.updateChannelInfo(channel)
	}

	if ch.NotExist {
		return nil
	}
	return ch
}

[dfc (Dave Cheney)]

1. Make sure you have high code coverage of this function then run the race detector.

2. I fear you have cargo culted this pattern. If it did what the comment suggests it did then there would be a huge bug in the compiler.

                // new stack frame to protect against panic causing double-unlock
		func() {
			c.channelLock.RUnlock()
			defer c.channelLock.RLock()
			_, _ = c.updateChannelInfo(channel)
		}()

[nathankerr]

I think the main thing making it messy are the two, equivalent if statements. Combining them really helps. Also, running the synchronous update in a closure doesn’t help. Instead, manually order the unlock/lock. Applying these gives:

func (c *DataCache) ChannelInfo(channel ChannelID) *Channel {
	c.channelLock.RLock()
	defer c.channelLock.RUnlock()

	ch, ok := c.channelInfo[channel]
	if !ok {
		c.channelLock.RUnlock()
		c.updateChannelInfo(channel)
		c.channelLock.RLock()

		ch, ok = c.channelInfo[channel]
		if !ok {
			return nil
		}
	} else if ch.CacheTS.Before(time.Now().Add(-24 * time.Hour)) {
		go c.updateChannelInfo(channel)
	}

	if ch.NotExist {
		return nil
	}

	return ch
}