GitLab Redirects

[ciyer]

No description provided.

[leafty]

NOTE: we should open an issue to replace this with code generated from the API spec.

[leafty]

The redirect map should not be public:

Suggested change

	RedirectMap map[string]RedirectStoreRedirectEntry
	redirectMap map[string]RedirectStoreRedirectEntry

[ciyer]

Fixed in 959944f

[leafty]

The current code does not protect bad reads of the RedirectMap from happening.

Since the RedirectMap is only ever accessed in this method, we can use the simple sync.Mutex and we should lock the read+update access:

Suggested change

	entry, ok := rs.RedirectMap[key]
	if ok && entry.UpdatedAt.Add(rs.EntryTtl).After(time.Now()) {
	return &entry, nil
	}
	rs.redirectMapMutex.Lock()
	defer rs.redirectMapMutex.Unlock()
	// Re-check after acquiring the lock, since it might have been updated meanwhile
	entry, ok = rs.RedirectMap[key]
	if !ok || entry.UpdatedAt.Add(rs.EntryTtl).Before(time.Now()) {
	rs.redirectMapMutex.Lock()
	defer rs.redirectMapMutex.Unlock()
	entry, ok := rs.RedirectMap[key]
	if ok && entry.UpdatedAt.Add(rs.EntryTtl).After(time.Now()) {
	return &entry, nil
	}
	if !ok || entry.UpdatedAt.Add(rs.EntryTtl).Before(time.Now()) {

Doing so will make sure that:

• Reads do not happen while the map is being updated
• This map will not be updated multiple times at once

The downside of this simple approach is that it will have poor read performance (full lock when just reading the map).

To enable better read performance, you can use a sync.RWMutex and protect reads and writes separately:

Suggested change

	entry, ok := rs.RedirectMap[key]
	if ok && entry.UpdatedAt.Add(rs.EntryTtl).After(time.Now()) {
	return &entry, nil
	}
	rs.redirectMapMutex.Lock()
	defer rs.redirectMapMutex.Unlock()
	// Re-check after acquiring the lock, since it might have been updated meanwhile
	entry, ok = rs.RedirectMap[key]
	if !ok || entry.UpdatedAt.Add(rs.EntryTtl).Before(time.Now()) {
	rs.redirectMapMutex.RLock()
	entry, ok := rs.RedirectMap[key]
	rs.redirectMapMutex.RUnlock()
	if ok && entry.UpdatedAt.Add(rs.EntryTtl).After(time.Now()) {
	return &entry, nil
	}
	rs.redirectMapMutex.Lock()
	defer rs.redirectMapMutex.Unlock()
	// Re-check after acquiring the lock, since it might have been updated meanwhile
	entry, ok = rs.RedirectMap[key]
	if !ok || entry.UpdatedAt.Add(rs.EntryTtl).Before(time.Now()) {

Here, we can have as many read locks as we want, which corresponds to the read-heavy model of this map.

[ciyer]

Fixed in e98ef85

[leafty]

LGTM