proposal: sync: add (*WaitGroup).WaitUnlock(l Locker)

[zhiqiangxu]

The passed in Locker will be Unlocked immediately if no need to wait, or right before wait(runtime_Semacquire).

This can be useful when some resources can be released before wait, e.g, when Add/Wait are synchronised by RWMutex,
where Add only happens while holding RLock and Wait only happens while holding Lock.

Here's a concrete example:

https://github.com/zhiqiangxu/util/blob/470c7893cce57bd749d95f66cd116e8880eaa068/closer/strict.go#L69

[ianlancetaylor]

I think you are looking for condition variables.

[zhiqiangxu]

@ianlancetaylor I've re-implemented it with sync.Cond in the latest code: https://github.com/zhiqiangxu/util/blob/3ab9e7087051ba225bfcc128bb28ca169383360e/closer/strict.go

But I think it's also useful for sync.WaitGroup, what do you think?

I've also changed WaitRelease to WaitUnlock, to be more explicit.

[ianlancetaylor]

I think it's problematic to add a user callback to lock heavy code like sync.WaitGroup. If the user callback fails to return or blocks while other code is executing and using the WaitGroup then the WaitGroup could easily get into in an inconsistent state. In the Go standard library we generally avoid callbacks--offhand I can't think of any callbacks at all, unless you count methods like String--and in this case I think it would be easy for the callback to be misused. The basic functionality is available through other data structures; that's why I mentioned condition variables. So personally I don't think this would be a good change to make in the Go standard library.

[zhiqiangxu]

I think it's problematic to add a user callback to lock heavy code like sync.WaitGroup. If the user callback fails to return or blocks while other code is executing and using the WaitGroup then the WaitGroup could easily get into in an inconsistent state. In the Go standard library we generally avoid callbacks--offhand I can't think of any callbacks at all, unless you count methods like String--and in this case I think it would be easy for the callback to be misused. The basic functionality is available through other data structures; that's why I mentioned condition variables. So personally I don't think this would be a good change to make in the Go standard library.

Yes, that's why I've changed WaitRelease to WaitUnlock in order to reduce the possibility of misuse. The tricky part in my case is that when Signal(), I need to take care so that Wait() doesn't miss it(there's a follow up fix for that), which is taken care of inside WaitGroup, which I'd like to reuse.

Regarding callbacks, in fact Cond also has a Locker which it Unlock internally before runtime_notifyListWait.

[bcmills]

In the linked example, why do you need the call to Wait to occur before s.mu.Unlock()?

It looks like you're already ensuring that s.waiting.Add is not called with a positive delta after that point.

[bcmills]

It seems like you could close a channel here instead of signaling a sync.Cond. Then it doesn't matter whether the lock is held, because no signal can possibly be dropped.

[zhiqiangxu]

It seems like you could close a channel here instead of signaling a sync.Cond. Then it doesn't matter whether the lock is held, because no signal can possibly be dropped.

That line may be called multiple times under race conditions.

[bcmills]

That line may be called multiple times under race conditions.

Sure, but it would be trivial to use an exclusive lock (instead of a read-lock) and select-with-default to eliminate that race.

[zhiqiangxu]

That line may be called multiple times under race conditions.

Sure, but it would be trivial to use an exclusive lock (instead of a read-lock) and select-with-default to eliminate that race.

Some demo code？ ：）

Anyway this proposal is just about code-reuse，not some lack of mechanics：）

The link in the OP shows how code can be simplified with it

[rsc]

In general the things in package sync are meant to stand alone. Synchronization code is subtle, and adding callbacks like this or mixing operations on one synchronization type with another lead to bugs. There are also ways to write the given code without the new API.

Based on the discussion above, this seems like a likely decline.

Leaving open for a week for final comments.

[rsc]

No change in consensus, so declining.