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 Mutex.LockContext #40026

Closed
nhooyr opened this issue Jul 3, 2020 · 8 comments
Closed

proposal: sync: add Mutex.LockContext #40026

nhooyr opened this issue Jul 3, 2020 · 8 comments
Labels
Projects
Milestone

Comments

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Jul 3, 2020

I propose we add the following function to the sync package:

// LockContext is like Lock but if the ctx is cancelled before
// the mutex is available, then it returns ctx.Error().
//
// It returns with nil if the mutex was successfully locked.
func (m *Mutex) LockContext(ctx context.Context) error {
    // ...
}

At the moment, you can mimic this your own mutex based on make(chan struct{}, 1).

Every lock would correspond to a select on the ctx done chan and a send on the mutex channel.

And an unlock would read the value out of the channel.

This works and is what I use in my websocket library but it might be more generally useful and deserve a spot in the standard library.

I've often found myself needing it when I want to acquire a resource but the resource may be acquired for somewhat long periods of time by another caller and so in order to keep my code responsive, I need any mutex Lock calls to respect the context.

@gopherbot gopherbot added this to the Proposal milestone Jul 3, 2020
@gopherbot gopherbot added the Proposal label Jul 3, 2020
@josharian
Copy link
Contributor

@josharian josharian commented Jul 3, 2020

Related: #27544 #6123

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jul 3, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 3, 2020

Personally I think the channel based mutex is a better choice here. sync.Mutex is most effective for locks that are held for a short period of time. I don't see how to implement this feature without complicating and slowing down sync.Mutex, which seems like a bad tradeoff for most programs.

@nhooyr
Copy link
Contributor Author

@nhooyr nhooyr commented Jul 4, 2020

If that's the case, I agree it's not a good idea to add it into sync.Mutex.

Perhaps it'd be better as a new ContextMutex type? We could just use channels under the hood to avoid a complex implementation. The channel approach while works isn't very discoverable or semantic.

@josharian
Copy link
Contributor

@josharian josharian commented Jul 4, 2020

cc @bcmills who has thought about this a lot

A package that wraps up some of the channel-based approaches sounds useful, although it could live outside the standard library.

@nhooyr
Copy link
Contributor Author

@nhooyr nhooyr commented Jul 4, 2020

A package that wraps up some of the channel-based approaches sounds useful, although it could live outside the standard library.

Agreed.

What other patterns do you think could be in this package? Others that come to my mind require generics.

@rsc rsc changed the title proposal: sync: Add Mutex.LockContext proposal: sync: add Mutex.LockContext Jul 8, 2020
@rsc rsc moved this from Incoming to Active in Proposals Jul 8, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jul 15, 2020

Based on the discussion above, this proposal - add Mutex.LockContext - seems like a likely decline.
(It would still be interesting to see a channel-based mutex helper package outside the standard library.)

@rsc rsc moved this from Active to Likely Decline in Proposals Jul 15, 2020
@nhooyr
Copy link
Contributor Author

@nhooyr nhooyr commented Jul 16, 2020

Agreed. I've created https://github.com/nhooyr/ctxsync but haven't implemented it yet.

Will close this for now. Feel free to watch the repo for updates.

@nhooyr nhooyr closed this Jul 16, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Jul 22, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jul 22, 2020

Retracted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.