sync: mutex.TryLock #6123

Closed
lukescott opened this Issue Aug 12, 2013 · 20 comments

Comments

Projects
None yet
@lukescott
This has come up several times on the mailing list. Looking for something like this to
be provided in sync.Mutex:

func (m *Mutex) TryLock() (locked bool) {
    locked = atomic.CompareAndSwapInt32(&m.state, 0, mutexLocked)
    if locked && raceenabled {
        raceAcquire(unsafe.Pointer(m))
    }
    return
}

Now you could use CompareAndSwapInt32 yourself. But it gets tricky when you need to use
Lock in one function, and TryLock in another on the same mutex.

The typical use-case is something like this:

if m.TryLock() {
    // do something
} else {
    // do something else
}

It's very similar to select default for channels:

select {
case <-c:
// do something
default:
// do something else
}

The difference being that with such a function you can do this with a primitive.

Other languages provide this functionality:

Java:
http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Lock.html#tryLock()
Objective-C:
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSLock_Class/Reference/Reference.html#//apple_ref/occ/instm/NSLock/tryLock

A "LockBeforeTime" would also be useful, but I understand that's more
difficult to implement.
@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 13, 2013

Contributor

Comment 1:

In the (I hope unlikely) event we decide to add this, please don't call it TryLock, a
poor name for a boolean-valued function.  (You tried, yes, but did you get it?)  Maybe
CanLock or GetLock; there's probably a better name I'm not thinking of.
Arguments that Java calls it tryLock butter none of my parsnips.

Labels changed: added priority-someday, removed priority-triage, go1.2maybe.

Contributor

robpike commented Aug 13, 2013

Comment 1:

In the (I hope unlikely) event we decide to add this, please don't call it TryLock, a
poor name for a boolean-valued function.  (You tried, yes, but did you get it?)  Maybe
CanLock or GetLock; there's probably a better name I'm not thinking of.
Arguments that Java calls it tryLock butter none of my parsnips.

Labels changed: added priority-someday, removed priority-triage, go1.2maybe.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 13, 2013

Contributor

Comment 2:

Status changed to Accepted.

Contributor

robpike commented Aug 13, 2013

Comment 2:

Status changed to Accepted.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 13, 2013

Contributor

Comment 3:

Labels changed: added feature.

Contributor

robpike commented Aug 13, 2013

Comment 3:

Labels changed: added feature.

@lukescott

This comment has been minimized.

Show comment
Hide comment
@lukescott

lukescott Aug 13, 2013

Comment 4:

"TryLock" or "GetLock" is fine. I don't care what you call it. "CanLock" says "Can I
Lock" not "Get a Lock if you can".
Java, Ruby, C++ (std::), and Objective-C have a TryLock function (and call it that),
just to name a few. Python has a single `Acquire(block bool) bool` function.
Why do you not want this? The sync.Mutex package is greatly limited compared to other
languages. Just because Go has channels doesn't mean the sync.Mutex package should be
left incomplete. Developers should be able to choose which to use: channel or mutex.
They shouldn't be forced to use channels or fend for themselves.

Comment 4:

"TryLock" or "GetLock" is fine. I don't care what you call it. "CanLock" says "Can I
Lock" not "Get a Lock if you can".
Java, Ruby, C++ (std::), and Objective-C have a TryLock function (and call it that),
just to name a few. Python has a single `Acquire(block bool) bool` function.
Why do you not want this? The sync.Mutex package is greatly limited compared to other
languages. Just because Go has channels doesn't mean the sync.Mutex package should be
left incomplete. Developers should be able to choose which to use: channel or mutex.
They shouldn't be forced to use channels or fend for themselves.
@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Aug 13, 2013

Contributor

Comment 5:

It's possible that channels are better suited to the kind of problems that you might
solve with TryLock in other languages.
As a data point, I've never even thought to use TryLock in Go code, and I've written a
lot of it.
What do you want to use it for?
Contributor

adg commented Aug 13, 2013

Comment 5:

It's possible that channels are better suited to the kind of problems that you might
solve with TryLock in other languages.
As a data point, I've never even thought to use TryLock in Go code, and I've written a
lot of it.
What do you want to use it for?
@lukescott

This comment has been minimized.

Show comment
Hide comment
@lukescott

lukescott Aug 13, 2013

Comment 6:

Some problems yes, but not all of them. Many of the issues I've had where channels
aren't appropriate are buffers. I'm requesting this in behalf of others, so they would
have to comment on their specific needs.
To give a more specific example in my case, LockBefore(Time) function would be useful in
the case of:
http://golang.org/src/pkg/io/pipe.go
http://golang.org/src/pkg/net/pipe.go
In implementing SetReadDeadline and SetWriteDeadline.
I believe that sync.Mutex should be more in parity with other languages; Just because a
channel can do something doesn't mean sync.Mutex should be left incomplete. I would
rather be given the choice rather than no choice at all.
TryLock/GetLock/Acquire (or whatever you call it) should be easy to implement. So there
is really no reason why it shouldn't be. LockBefore(Time) should be implemented to, but
I understand that introducing a timed delay is more complex with the scheduler.

Comment 6:

Some problems yes, but not all of them. Many of the issues I've had where channels
aren't appropriate are buffers. I'm requesting this in behalf of others, so they would
have to comment on their specific needs.
To give a more specific example in my case, LockBefore(Time) function would be useful in
the case of:
http://golang.org/src/pkg/io/pipe.go
http://golang.org/src/pkg/net/pipe.go
In implementing SetReadDeadline and SetWriteDeadline.
I believe that sync.Mutex should be more in parity with other languages; Just because a
channel can do something doesn't mean sync.Mutex should be left incomplete. I would
rather be given the choice rather than no choice at all.
TryLock/GetLock/Acquire (or whatever you call it) should be easy to implement. So there
is really no reason why it shouldn't be. LockBefore(Time) should be implemented to, but
I understand that introducing a timed delay is more complex with the scheduler.
@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Aug 13, 2013

Contributor

Comment 7:

Thanks for the response, and I can see where you're coming from.
But:
1. My question was directly about TryLock, and you responded with arguments for
LockBefore.
2. Every feature is useful. If it wasn't useful, we wouldn't be talking about it. Go is
respected both for what it provides, and also for what it leaves out. It doesn't matter
if the feature is easy to implement. Often "it is easy to do" is a very bad reason to
add features.
This is not me arguing against TryLock. I have no strong opinion one way or another.
Rather, I am trying to find the argument *for* TryLock.
Contributor

adg commented Aug 13, 2013

Comment 7:

Thanks for the response, and I can see where you're coming from.
But:
1. My question was directly about TryLock, and you responded with arguments for
LockBefore.
2. Every feature is useful. If it wasn't useful, we wouldn't be talking about it. Go is
respected both for what it provides, and also for what it leaves out. It doesn't matter
if the feature is easy to implement. Often "it is easy to do" is a very bad reason to
add features.
This is not me arguing against TryLock. I have no strong opinion one way or another.
Rather, I am trying to find the argument *for* TryLock.
@lukescott

This comment has been minimized.

Show comment
Hide comment
@lukescott

lukescott Aug 13, 2013

Comment 8:

There's an ongoing topic on the mailing list about it. I posted a link, so hopefully
others can give you a more concrete examples. There have also been other topics about it
in the past, and I'm sure it will come up again and again.
The only example I can think of off the top of my head: When protecting a value, like a
buffer, that you would otherwise use a sync.Mutex/sync.RWMutex, and you wanted to have a
blocking (uses Lock()) and non-blocking call (uses TryLock()). A situation where you'd
end up doing something like this:
lock := make(chan bool)
...
select {
case <-lock:
// do something
default:
    return errors.New("Busy")
}
When this would have been much more concise:
if m.TryLock() == false {
    return errors.New("Busy")
}
// do something
Channels are also harder in this situation when you need a sync.RWMutex.

Comment 8:

There's an ongoing topic on the mailing list about it. I posted a link, so hopefully
others can give you a more concrete examples. There have also been other topics about it
in the past, and I'm sure it will come up again and again.
The only example I can think of off the top of my head: When protecting a value, like a
buffer, that you would otherwise use a sync.Mutex/sync.RWMutex, and you wanted to have a
blocking (uses Lock()) and non-blocking call (uses TryLock()). A situation where you'd
end up doing something like this:
lock := make(chan bool)
...
select {
case <-lock:
// do something
default:
    return errors.New("Busy")
}
When this would have been much more concise:
if m.TryLock() == false {
    return errors.New("Busy")
}
// do something
Channels are also harder in this situation when you need a sync.RWMutex.
@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Aug 13, 2013

Contributor

Comment 9:

Your example is still too abstract. It shows the operation of TryLock, but not the
surrounding problem, and so it is not convincing. This issue is the correct place to
make the case for the addition of TryLock. I await further responses.
Contributor

adg commented Aug 13, 2013

Comment 9:

Your example is still too abstract. It shows the operation of TryLock, but not the
surrounding problem, and so it is not convincing. This issue is the correct place to
make the case for the addition of TryLock. I await further responses.
@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Aug 13, 2013

Contributor

Comment 10:

IMHO TryLock (let me stick to the OP's proposed name to not add confusion) is a
reasonable thing to have. But only in a classical, old school world of threads. Threads
are expensive. Both to create and to switch context to. Whenever there's an option to
blocking such expensive thing and do whatever useful instead, then it buys better
resource allocation - hopefully. However, this is sort of a manual scheduling. Or at
least scheduling "hint".
In the "new" world composed of goroutines the above doesn't hold anymore. Goroutines are
cheap. Both to create and to switch context to. When a goroutine gets blocked, no drama
happens. Typically there are other goroutines in the ready state available for
scheduling. If not, the process sleeps, but that's what good processes are expected to
do when idle b/c of blocked on I/O, timer, ...
Also, "explicit" scheduling by blocking on mutexes or channel operations is more easily
reasoned about than is the case of a goroutine which tries to get a lock, in which case
it does Foo, but if that fails it performs Bar.
Conclusion: I think that adding TryLock encourages programming practices obsolete from
the Go execution model POV (like eg. busy waiting). Make your goroutines simple; doing
one thing and doing it well. It'll save you headaches while bug hunting.
-1 from me.
Contributor

cznic commented Aug 13, 2013

Comment 10:

IMHO TryLock (let me stick to the OP's proposed name to not add confusion) is a
reasonable thing to have. But only in a classical, old school world of threads. Threads
are expensive. Both to create and to switch context to. Whenever there's an option to
blocking such expensive thing and do whatever useful instead, then it buys better
resource allocation - hopefully. However, this is sort of a manual scheduling. Or at
least scheduling "hint".
In the "new" world composed of goroutines the above doesn't hold anymore. Goroutines are
cheap. Both to create and to switch context to. When a goroutine gets blocked, no drama
happens. Typically there are other goroutines in the ready state available for
scheduling. If not, the process sleeps, but that's what good processes are expected to
do when idle b/c of blocked on I/O, timer, ...
Also, "explicit" scheduling by blocking on mutexes or channel operations is more easily
reasoned about than is the case of a goroutine which tries to get a lock, in which case
it does Foo, but if that fails it performs Bar.
Conclusion: I think that adding TryLock encourages programming practices obsolete from
the Go execution model POV (like eg. busy waiting). Make your goroutines simple; doing
one thing and doing it well. It'll save you headaches while bug hunting.
-1 from me.
@dvyukov

This comment has been minimized.

Show comment
Hide comment
@dvyukov

dvyukov Aug 13, 2013

Member

Comment 11:

We need very strong use cases to add this. If they would exist, we've most likely
noticed them already.
As 0xjnml said, pthread/C/Java primitives are designed for very different environment
and programming model.
I can imagine a rare case for only trylocked mutex along the lines of:
for ... {
  // do something local
  if atomic.SwapUint32(&merger, 1) == 0 {
    // merge local results into global results
    atomic.StoreUint32(&merger, 0)
  }
}
But this is easily solved with atomic operations.
As for TimedLock, mutex is the very wrong level to solve priorities/deadlines/timeouts.
#WontFix
Member

dvyukov commented Aug 13, 2013

Comment 11:

We need very strong use cases to add this. If they would exist, we've most likely
noticed them already.
As 0xjnml said, pthread/C/Java primitives are designed for very different environment
and programming model.
I can imagine a rare case for only trylocked mutex along the lines of:
for ... {
  // do something local
  if atomic.SwapUint32(&merger, 1) == 0 {
    // merge local results into global results
    atomic.StoreUint32(&merger, 0)
  }
}
But this is easily solved with atomic operations.
As for TimedLock, mutex is the very wrong level to solve priorities/deadlines/timeouts.
#WontFix
@nigeltao

This comment has been minimized.

Show comment
Hide comment
@nigeltao

nigeltao Aug 14, 2013

Contributor

Comment 13:

Most recent mailing list discussion:
https://groups.google.com/d/topic/golang-nuts/vfbEGJCHGXM/discussion
Contributor

nigeltao commented Aug 14, 2013

Comment 13:

Most recent mailing list discussion:
https://groups.google.com/d/topic/golang-nuts/vfbEGJCHGXM/discussion
@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 27, 2013

Contributor

Comment 14:

Labels changed: added go1.3maybe.

Contributor

rsc commented Nov 27, 2013

Comment 14:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 27, 2013

Contributor

Comment 15:

Labels changed: removed feature.

Contributor

rsc commented Nov 27, 2013

Comment 15:

Labels changed: removed feature.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 16:

Labels changed: added release-none, removed go1.3maybe.

Contributor

rsc commented Dec 4, 2013

Comment 16:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 17:

Labels changed: added repo-main.

Contributor

rsc commented Dec 4, 2013

Comment 17:

Labels changed: added repo-main.

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Dec 18, 2014

Contributor

It's been more than a year since the last comment on this issue, and nobody has provided a compelling argument for this feature. I don't think it's going to happen.

Contributor

adg commented Dec 18, 2014

It's been more than a year since the last comment on this issue, and nobody has provided a compelling argument for this feature. I don't think it's going to happen.

@adg adg closed this Dec 18, 2014

@beatgammit

This comment has been minimized.

Show comment
Hide comment
@beatgammit

beatgammit Mar 31, 2015

In case someone else comes along here, I think this would work for a general-purpose non-blocking semaphore:

select {
    case v := <-tryLockSem:
        // do something
        tryLockSem <- v
    default:
        // did not get the lock
}

It's not as efficient as it could be, but it's very simple and easily understandable as it uses basic language features (and is flexible enough to allow any type in the channel).

In case someone else comes along here, I think this would work for a general-purpose non-blocking semaphore:

select {
    case v := <-tryLockSem:
        // do something
        tryLockSem <- v
    default:
        // did not get the lock
}

It's not as efficient as it could be, but it's very simple and easily understandable as it uses basic language features (and is flexible enough to allow any type in the channel).

@alkemir

This comment has been minimized.

Show comment
Hide comment
@alkemir

alkemir May 1, 2015

How about creating a pool of resources (say connections or buffers) ?

Code would look like this:

func (pool *Pool) getResource() {
   for _, resource := range pool {
      if resource.lock.Try() {
         return resource
      }
   }

   pool.Lock()
   if pool.len() < MAX_POOL_SIZE {
      r := NewResource()
      pool.add(r)
      pool.Unlock()
      return r
   }

   pool.Unlock()
   // Stand in the waitgroup for resources to be freed
   // and return it
}

This is like sync.Pool but for resources that you don't want deallocated when free.
I guess the alternative using channels would have the resources send themselves
over a channel when free? Something like this:

func (pool *Pool) getResource() {
   select {
      case v := <-pool.freeResources
         return v 
      default:
         pool.lock.Lock()                           // Lock needed to honor MAX_POOL_SIZE
         refer pool.lock.Unlock()
         if pool.length < MAX_POOL_SIZE {
            pool.length++
            return NewResource()
        }
        return <- pool.freeResources
}

func (pool *Pool) freeResource(r *Resource) {
   pool.freeResources <- r
}

The good side of using channels is that the "WaitGroup logic" is straightforward. Also, you dont need to iterate over the busy resources. The only disadvantage with using channels that I can see is that freeing a resource is locking if you reach channel capacity. But then again, you can always call freeResource in a goroutine if you can't estimate the buffering you are going to need.

Although this would seem weird, considering you have MAX_POOL_SIZE, think of a case when MAX_POOL_SIZE is a soft limit, that is, you have two different calls for adquiring resources; getResource() and getResourceNow(). The second method does not honor the limit and is used on crucial situations when you cannot be bothered by that limit.

Would this pattern be correct?

alkemir commented May 1, 2015

How about creating a pool of resources (say connections or buffers) ?

Code would look like this:

func (pool *Pool) getResource() {
   for _, resource := range pool {
      if resource.lock.Try() {
         return resource
      }
   }

   pool.Lock()
   if pool.len() < MAX_POOL_SIZE {
      r := NewResource()
      pool.add(r)
      pool.Unlock()
      return r
   }

   pool.Unlock()
   // Stand in the waitgroup for resources to be freed
   // and return it
}

This is like sync.Pool but for resources that you don't want deallocated when free.
I guess the alternative using channels would have the resources send themselves
over a channel when free? Something like this:

func (pool *Pool) getResource() {
   select {
      case v := <-pool.freeResources
         return v 
      default:
         pool.lock.Lock()                           // Lock needed to honor MAX_POOL_SIZE
         refer pool.lock.Unlock()
         if pool.length < MAX_POOL_SIZE {
            pool.length++
            return NewResource()
        }
        return <- pool.freeResources
}

func (pool *Pool) freeResource(r *Resource) {
   pool.freeResources <- r
}

The good side of using channels is that the "WaitGroup logic" is straightforward. Also, you dont need to iterate over the busy resources. The only disadvantage with using channels that I can see is that freeing a resource is locking if you reach channel capacity. But then again, you can always call freeResource in a goroutine if you can't estimate the buffering you are going to need.

Although this would seem weird, considering you have MAX_POOL_SIZE, think of a case when MAX_POOL_SIZE is a soft limit, that is, you have two different calls for adquiring resources; getResource() and getResourceNow(). The second method does not honor the limit and is used on crucial situations when you cannot be bothered by that limit.

Would this pattern be correct?

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor May 1, 2015

Contributor

If you are asking a question, please ask on the mailing list golang-nuts@googlegroups.com. Please don't ask questions on the issue tracker. And particularly please don't comment on closed issues; if you want to reopen one, bring that up on the mailing list as well. Thanks.

Contributor

ianlancetaylor commented May 1, 2015

If you are asking a question, please ask on the mailing list golang-nuts@googlegroups.com. Please don't ask questions on the issue tracker. And particularly please don't comment on closed issues; if you want to reopen one, bring that up on the mailing list as well. Thanks.

@golang golang locked and limited conversation to collaborators Jul 18, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.