Navigation Menu

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

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

Closed
zhiqiangxu opened this issue Jan 1, 2020 · 11 comments
Closed

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

zhiqiangxu opened this issue Jan 1, 2020 · 11 comments

Comments

@zhiqiangxu
Copy link
Contributor

zhiqiangxu commented Jan 1, 2020

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

@zhiqiangxu zhiqiangxu changed the title sync.WaitGroup: add WaitRelease(releaseFunc func()) Proposal: sync.WaitGroup: add WaitRelease(releaseFunc func()) Jan 1, 2020
@gopherbot gopherbot added this to the Proposal milestone Jan 1, 2020
@ianlancetaylor ianlancetaylor changed the title Proposal: sync.WaitGroup: add WaitRelease(releaseFunc func()) proposal: sync: add (*WaitGroup).WaitRelease(releaseFunc func()) Jan 1, 2020
@ianlancetaylor
Copy link
Contributor

I think you are looking for condition variables.

zhiqiangxu added a commit to zhiqiangxu/go that referenced this issue Jan 2, 2020
The passed in `releaseFunc` will be called 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/master/closer/state.go#L58

Fixed golang#36349
zhiqiangxu added a commit to zhiqiangxu/go that referenced this issue Jan 2, 2020
The passed in `Locker` will be called 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/master/closer/state.go#L58

Fixed golang#36349
@zhiqiangxu
Copy link
Contributor Author

@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.

@zhiqiangxu zhiqiangxu changed the title proposal: sync: add (*WaitGroup).WaitRelease(releaseFunc func()) proposal: sync: add (*WaitGroup).WaitUnlock(l Locker) Jan 2, 2020
@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor Author

zhiqiangxu commented Jan 2, 2020

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
Copy link
Contributor

bcmills commented Jan 7, 2020

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
Copy link
Contributor

bcmills commented Jan 7, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

bcmills commented Jan 8, 2020

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
Copy link
Contributor Author

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 rsc added this to Incoming in Proposals (old) Jan 8, 2020
@rsc
Copy link
Contributor

rsc commented Jan 8, 2020

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 rsc moved this from Incoming to Likely Decline in Proposals (old) Jan 8, 2020
@rsc
Copy link
Contributor

rsc commented Jan 15, 2020

No change in consensus, so declining.

@rsc rsc closed this as completed Jan 15, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jan 15, 2020
@golang golang locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants