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.

Sign up for GitHub

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

Jump to bottom

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
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@nhooyr
Copy link
Contributor

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

Related: #27544 #6123

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

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

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 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 (old) Jul 8, 2020
@rsc rsc mentioned this issue Jul 8, 2020
Open
@rsc
Copy link
Contributor

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 (old) Jul 15, 2020
@nhooyr
Copy link
Contributor Author

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 as completed Jul 16, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jul 22, 2020
@rsc
Copy link
Contributor

rsc commented Jul 22, 2020

Retracted.

@golang golang locked and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
5 participants
@josharian @rsc @ianlancetaylor @gopherbot @nhooyr