Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sync: add new Map methods CompareAndSwap, CompareAndDelete, Swap #51972

Open
9072997 opened this issue Mar 27, 2022 · 13 comments
Open

sync: add new Map methods CompareAndSwap, CompareAndDelete, Swap #51972

9072997 opened this issue Mar 27, 2022 · 13 comments

Comments

@9072997
Copy link

@9072997 9072997 commented Mar 27, 2022

I propose adding a new method on sync.Map similar to atomic.Value's Swap() method. I think either the name LoadAndStore() to match the existing LoadOrStore or Swap() to match atomic.Value would make sense. The function signature would look something like this

func (m *Map) LoadAndStore(key, newValue any) (previous any, loaded bool)

I think the fact that this already exists in atomic.Value is a good argument that there is a use case for it. The same thing could be achieved by creating a sync.Map of atomic.Values, but that is a lot of type-assertion, and I have to stare at it pretty hard to make sure it's free of race conditions. My specific use case is basically de-bouncing abuse reports. If a worker detects abuse from a client it would

lastReport, hasReportHistory := lastReportTimes.LoadAndStore(clientIP, time.Now())
if hasReportHistory && time.Since(lastReport.(time.Time)) < time.Hour {
    log("not re-sending abuse report")
    return
}
sendAbuseReport(clientIP)
@gopherbot gopherbot added this to the Proposal milestone Mar 27, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 27, 2022

CC @bcmills

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 27, 2022
@rsc rsc changed the title proposal: sync: add new Map method LoadAndStore proposal: sync: add new Map method Swap Mar 30, 2022
@rsc rsc moved this from Incoming to Active in Proposals Mar 30, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Mar 30, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

@rsc rsc commented Apr 6, 2022

@bcmills, what do you think?

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 6, 2022

func (m *Map) Swap(key, value any) (previous any, loaded bool) seems like a reasonable addition to me.

It has the right granularity to fit with the other Map methods (only synchronizes access to a specific key, and has similar atomicity properties to LoadOrStore), and it enables coding patterns that are otherwise complicated or inefficient.

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 6, 2022

We may also want to consider CompareAndSwap and CompareAndDelete at the same time to parallel (*atomic.Value).CompareAndSwap, which was added along with Swap in proposal #39351. That might look like:

// CompareAndSwap swaps the old and new values for key
// if the value stored in the map is equal to old.
// The old value must be of a comparable type.
//
// If old is the nil interface value, the swap will occur if either there
// is no existing value for the key or the existing value is also the
// nil interface.
func (m *Map) CompareAndSwap(key, old, new any) (swapped bool)

// CompareAndDelete deletes the entry for key if its value is equal to old.
// The old value must be of a comparable type.
//
// If there is no current value for key in the map, CompareAndDelete
// returns false (even if the old value is the nil interface value).
func (m *Map) CompareAndDelete(key, old any) (deleted bool)

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2022

Change https://go.dev/cl/399094 mentions this issue: sync: add new Map method Swap

@rsc rsc changed the title proposal: sync: add new Map method Swap proposal: sync: add new Map methods CompareAndSwap, CompareAndDelete, Swap Apr 13, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Apr 13, 2022

Does anyone object to adding Swap, CompareAndSwap, and CompareAndDelete?

@changkun
Copy link
Member

@changkun changkun commented Apr 14, 2022

While prototyping the CompareAndDelete, it got me thinking: is it really make sense to have CompareAndDelete?

sync.Map has two maps inside and is optimized for read-most context.

Since we don't have an atomic compare and delete primitive. What can we do to the read map to compare the old value, and delete it atomically? I could not yet make sense out of this implementation:

func (m *Map) CompareAndDelete(key, old any) (deleted bool) {
	read, _ := m.read.Load().(readOnly)
	if e, ok := read.m[key]; ok {
		if e.tryCompareAndSwap(&old, nil) {
			// Here is clearly(?) non-atomic
			// what if something happens between the above CAS and below delete?
			// Is it possible?
			delete(read.m, key)
			deleted = true
			return
		}
	}

	m.mu.Lock()
	...
}

Or is this something I am not going in the right direction of implementation? It looks like to me that we have to lock the entire Map to proceed? Am I overthinking here? What is the fast path in this case?

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 18, 2022

@changkun, the use case for CompareAndDelete is to delete, say, stale entries from a cache (which otherwise could have the right properties).

If the key is a hit in the read-only map, the entry will still remain but be marked as deleted.

@rsc
Copy link
Contributor

@rsc rsc commented May 4, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals May 4, 2022
@rsc rsc moved this from Likely Accept to Accepted in Proposals May 11, 2022
@rsc
Copy link
Contributor

@rsc rsc commented May 11, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: sync: add new Map methods CompareAndSwap, CompareAndDelete, Swap sync: add new Map methods CompareAndSwap, CompareAndDelete, Swap May 11, 2022
@rsc rsc removed this from the Proposal milestone May 11, 2022
@rsc rsc added this to the Backlog milestone May 11, 2022
@bcmills
Copy link
Member

@bcmills bcmills commented May 13, 2022

In #51972 (comment) I suggested that a CompareAndSwap wild old == nil should succeed if there is no existing key in the map. However, reading @changkun's implementation in https://go.dev/cl/399094 I think it probably makes more sense to never swap with a nonexistent key.

Here's my thinking: if we use a stricter CompareAndSwap that requires the key to exist, then the more lax semantics can be implemented in terms of that:

func StoreOrCompareAndSwapNil(m *sync.Map, key, new interface) (swapped bool) {
	old, loaded := m.LoadOrStore(key, new)
	if !loaded {
		return true  // Swapped new against “no key”.
	}
	if old != nil {
		return false  // Old value was not the nil interface value.
	}
	// Try to swap new against an explicit, existing nil.
	return m.CompareAndSwap(key, old, new)
}

The reverse does not hold, so the stricter CompareAndSwap seems more expressive overall.

Moreover, the Map operations need to know about explicit nil values for the other methods anyway, so it's not like the implementation gets any simpler if we use the lax semantics instead.

@bcmills
Copy link
Member

@bcmills bcmills commented May 13, 2022

The stricter CompareAndSwap (that never swaps against “no existing value”) also better matches CompareAndDelete, which never deletes a nonexistent value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Development

No branches or pull requests

6 participants