Additions to sync.Mutex

[Joshua Liebow-Feeser]

Recently, I found myself wanting to be able to inspect a sync.Mutex to find out if it was locked without actually blocking. Since this isn't possible without using unsafe, I came up with a workaround:

http://godoc.org/github.com/joshlf13/sync

However, that workaround works about 2x slower than the standard sync.Mutex. What do people think of adding that functionality (in particular, two methods: LockIfPossible, which does what it sounds like, and Locked, which checks the state of the mutex - neither blocks execution)?

Cheers,
Josh

[Dan Kortschak]

This has been asked for a number of times in the past and rejected
because it leads people to believe in things that are not justifiable.

Example:

if m.Locked() {
// Is m truly locked here?

[Joshua Liebow-Feeser]

That makes sense. What about a LockedIfPossible method, though? It behaves the same way as Lock if a lock can be obtained, otherwise it lets the user know that the lock is currently unobtainable, which, in the normal use cases, means "don't do that thing you were about to do."

[Daniel Barrett]

And if, when that function returns, it becomes available for a lock? Why not let blocks happen as they naturally would and work on other things in other go routines? You can do something like you want with the  select statement and channels if you need to choose what to do based on what's currently blocking.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Jesse McNelis]

On Mon, Aug 12, 2013 at 1:39 PM, Joshua Liebow-Feeser <josh...@gmail.com> wrote:

That makes sense. What about a LockedIfPossible method, though? It behaves the same way as Lock if a lock can be obtained, otherwise it lets the user know that the lock is currently unobtainable, which, in the normal use cases, means "don't do that thing you were about to do." 

LockedIfPossible has the same problem. You would need a lock to protect the lock while you're checking if it's locked.

Doing an action can be atomic but asking a question never is. 

You should structure your program around doing things and blocking until they are done.

If nothing ever blocks then synchronising is never guaranteed.

You could call LockedIfPossible endlessly and never get a lock even if the lock was being locked and unlocked in the mean time.

[Joshua Liebow-Feeser]

Using channels is actually much more complex. I'll give you an example (in fact, the use case that led me to the idea of LockIfPossible):

You have a package which exports a number of convenience functions. In the package, you have a global byte slice which you use as scratch space in some of the functions. In order to avoid a race condition, you want to lock access to the slice. However, forcing only one function to run at a time is not very performant - you'd like to have each function use the byte slice if possible, but if not, just allocate one for temporary use. I tried to figure out a scheme which used channels for this, but I couldn't come up with anything good. By contrast, when using LockIfPossible, the code is /really/ clean. Here's a paraphrased excerpt from my code:

func DoSomething() {
    if mutex.LockIfPossible {
        doSomethingWithBuffer()
    } else {
        doSomethingWithoutBuffer()

[Joshua Liebow-Feeser]

The use case (see my previous reply to Daniel Barret) is more - "if I can get the lock, great, otherwise I'll try a different way."

[Daniel Barrett]

You can do something like this example from the Effective Go documentation:

http://golang.org/doc/effective_go.html#leaky_buffer

It uses a channel along with select to either grab a free buffer, if it's available, or to allocate one.

[Joshua Liebow-Feeser]

I'll admit that that works well, but it does seem like a lot of code. It may work better in the case where there are multiple buffers, but if you just want one, LockIfPossible strikes me as a much simpler, cleaner, clearer use.

That aside, I also don't think that there's much of a reason /not/ to have LockIfPossible. Its semantics are relatively straightforward, and it's unlikely to be used incorrectly. Additionally, the common mantra in Go is "only use mutexes if you really know what you're doing," so even if the semantics are slightly more complex than for normal Lock, it fits in with the normal use case.

[Dan Kortschak]

A single chan Buffer with cap() == 1 does the job you want exactly.

http://play.golang.org/p/jURO5xqfcJ

[Joshua Liebow-Feeser]

That's true, but I still think it's cleaner with a single if statement. In any case, this is mostly a style argument at this point. I think a more relevant question is: what are the reasons /not/ to include LockIfPossible?

[Jan Mercl]

On Mon, Aug 12, 2013 at 6:34 AM, Joshua Liebow-Feeser
<josh...@gmail.com> wrote:
> That's true, but I still think it's cleaner with a single if statement. In
> any case, this is mostly a style argument at this point. I think a more
> relevant question is: what are the reasons /not/ to include LockIfPossible?

My answer is not going to be popular but: B/c there's no good reason
to include it.

BTW: You can write your own TryLock using the sync package primitives
quite easily (if you don't require a stand alone .Lock). Untested
pseudocode:

if atomic_increment (atomic_add 1 to) X == 1 {
perform "lock acquired" operation like reuse some buffer
atomic_decrement (atomic_add -1 to) X
} else {
perform "failed to lock" path like alloc a new buffer
}

Alt.: atomic_cmp_swap X to any non zero value when == 0 and atomic_set
X to zero instead of atomic +1/-1.

-j

[Dmitry Vyukov]

On Mon, Aug 12, 2013 at 7:29 AM, Dan Kortschak <dan.ko...@adelaide.edu.au> wrote:

This has been asked for a number of times in the past and rejected
because it leads people to believe in things that are not justifiable.

Example:

if m.Locked() {
        // Is m truly locked here?

}

Then we should delete time.Now()

now := time.Now()

// Is it still the same time here?

along with os.File.Stat, runtime.ReadMemStats, and pretty much the rest of the standard library.

[Dan Kortschak]

Yup, let's all go home.

[Volker Dobler]

Am Montag, 12. August 2013 10:19:43 UTC+2 schrieb Dmitry Vyukov:

On Mon, Aug 12, 2013 at 7:29 AM, Dan Kortschak <dan.ko...@adelaide.edu.au> wrote:

This has been asked for a number of times in the past and rejected
because it leads people to believe in things that are not justifiable.

Example:

if m.Locked() {
        // Is m truly locked here?

}

Then we should delete time.Now()

now := time.Now()

// Is it still the same time here?

A bad example considering that no one has the slightest idea

what time really is... (if it exists at all).

V. 

[Dan Kortschak]

Just to be clear, there are pretty significant differences between at least some of those and the lock case.

To make things worse, everything is in the past by the time you know about it. We manage, but when you intend to own something there is a fairly strong semantic imperative to know whether it is possible to own it.

On 12/08/2013, at 5:50 PM, "Dmitry Vyukov" <dvy...@google.com> wrote:

[Dmitry Vyukov]

On Mon, Aug 12, 2013 at 12:58 PM, Dan Kortschak <dan.ko...@adelaide.edu.au> wrote:

Just to be clear, there are pretty significant differences between at least some of those and the lock case.

To make things worse, everything is in the past by the time you know about it. We manage, but when you intend to own something there is a fairly strong semantic imperative to know whether it is possible to own it.

It depends on how one is going to use it.

IsFileExists() is useful in itself. But if you check that a file does not exists and then expect that file creation will succeed, that is wrong.

[Dan Kortschak]

Yes, context is everything. And the most likely context for asking about the status of a lock is a view to acquiring that lock.

[Dmitry Vyukov]

If there is TryLock (which is also proposed here), then you do not use IsLocked/Lock to try lock, you use TryLock to try lock.

[Dmitry Vyukov]

If you need only TryLock (no Lock), then it's trivial to implement with atomic operations:

// TryLock

return atomic.CompareAndSwapUint32(&mu, 0, 1)

// Unlock

atomic.StoreUint32(&mu, 0)

--

[Dan Kortschak]

Sure, but that's not actually what he needed and not what I answered. The language already gives him a way of safely requesting a reusable buffer, and maybe in the future he'll have access to a package in the standard library to do the same an more. ;)

[Jan Mercl]

On Mon, Aug 12, 2013 at 12:39 PM, Dan Kortschak
<dan.ko...@adelaide.edu.au> wrote:
> Sure, but that's not actually what he needed and not what I answered. The language already gives him a way of safely requesting a reusable buffer, and maybe in the future he'll have access to a package in the standard library to do the same an more. ;)

Apropos "safely requesting a reusable buffer": see 'CCache' and
'GCache' under http://godoc.org/github.com/cznic/bufs#CCache
</ad>

-j

[John Asmuth]

LockedIfPossible, or TryLock doesn't have the same problem because you are guaranteed to have progress. A general lock might never be unlocked, or might be idle for a long time. The lock surrounding a lock, as long as it's only accessed by the .TryLock() method, can actually be considered "lock-free" (I think), since whatever has that locked is guaranteed to make fast progress (it just checks the status of the other lock and locks it if possible).

On Monday, August 12, 2013 12:05:43 AM UTC-4, Jesse McNelis wrote:

[Joshua Liebow-Feeser]

That's my intuition. In practice, I implemented my mutex using an internal mutex to do Lock-Check-Unlock, but that's only because I didn't feel like copy+pasting the source out of sync.Mutex. To get perhaps overly technical, here's the source of sync.Mutex.Lock:

    41	func (m *Mutex) Lock() {
    42		// Fast path: grab unlocked mutex.
    43		if atomic.CompareAndSwapInt32(&m.state, 0, mutexLocked) {
    44			if raceenabled {
    45				raceAcquire(unsafe.Pointer(m))
    46			}
    47			return
    48		}
    49	
    50		awoke := false
    51		for {
    52			old := m.state
    53			new := old | mutexLocked
    54			if old&mutexLocked != 0 {
    55				new = old + 1<<mutexWaiterShift
    56			}
    57			if awoke {
    58				// The goroutine has been woken from sleep,
    59				// so we need to reset the flag in either case.
    60				new &^= mutexWoken
    61			}
    62			if atomic.CompareAndSwapInt32(&m.state, old, new) {
    63				if old&mutexLocked == 0 {
    64					break
    65				}
    66				runtime_Semacquire(&m.sema)
    67				awoke = true
    68			}
    69		}
    70	
    71		if raceenabled {
    72			raceAcquire(unsafe.Pointer(m))
    73		}
    74	}

You could easily modify that to not loop, and you'd get LockIfPossible. Something like this (haven't tested this):

    41	func (m *Mutex) LockIfPossible() bool {
    42		// Fast path: grab unlocked mutex.
    43		if atomic.CompareAndSwapInt32(&m.state, 0, mutexLocked) {
    44			if raceenabled {
    45				raceAcquire(unsafe.Pointer(m))
    46			}
    47			return true
    48		}
    48		return false
    49	}

That doesn't require any mechanisms beyond what sync.Mutex already has.

[Joshua Liebow-Feeser]

At this point, I'm more curious about adding LockIfPossible for other people; I know I could implement what I need myself, but this functionality might be useful for other people (otherwise adding it to the standard library wouldn't be appropriate).

[Luke Scott]

This is great if you're only doing a "TryLock". "Lock" and "TryLock" in different functions on the same mutex are a different story.

It should be as simple as this:

func (m *Mutex) TryLock() (locked bool) {

locked = atomic.CompareAndSwapInt32(&m.state, 0, mutexLocked)

if locked && raceenabled {

raceAcquire(unsafe.Pointer(m))

}

return

}

Luke

[Joshua Liebow-Feeser]

Exactly.

That said, there's no reason you can't have Lock and TryLock available on the same mutex. TryLock is just Lock without a loop. Multiple calls to Lock are /meant/ to be called at the same time. That's the whole point of a mutex. Thus, concurrent Lock and TryLock calls shouldn't be an issue.

[Dmitry Vyukov]

On Tue, Aug 13, 2013 at 12:13 AM, Luke Scott <lu...@visionlaunchers.com> wrote:

This is great if you're only doing a "TryLock". "Lock" and "TryLock" in different functions on the same mutex are a different story.

It should be as simple as this:

func (m *Mutex) TryLock() (locked bool) {

locked = atomic.CompareAndSwapInt32(&m.state, 0, mutexLocked)

if locked && raceenabled {

raceAcquire(unsafe.Pointer(m))

This race part is not necessary.  Users of the language implementation and the std lib are not intended to use them. It should just work.

[Jan Mercl]

On Mon, Aug 12, 2013 at 11:09 PM, Joshua Liebow-Feeser
<josh...@gmail.com> wrote:
> That said, there's no reason you can't have Lock and TryLock available on
> the same mutex. TryLock is just Lock without a loop.

That's strange. Lock should not loop (in the first approximation).

-j

[Joshua Liebow-Feeser]

The library implementation of sync.Mutex.Lock attempts to acquire the lock, and if it fails, loops attempting until it succeeds, like this:

Lock:
    if acquire():
        return
    loop:
        if acquire():
            return

Here's the actual Go code:

[Luke Scott]

On Monday, August 12, 2013 2:21:12 PM UTC-7, Dmitry Vyukov wrote:

On Tue, Aug 13, 2013 at 12:13 AM, Luke Scott <lu...@visionlaunchers.com> wrote:

This is great if you're only doing a "TryLock". "Lock" and "TryLock" in different functions on the same mutex are a different story.

It should be as simple as this:

func (m *Mutex) TryLock() (locked bool) {

locked = atomic.CompareAndSwapInt32(&m.state, 0, mutexLocked)

if locked && raceenabled {

raceAcquire(unsafe.Pointer(m))

This race part is not necessary.  Users of the language implementation and the std lib are not intended to use them. It should just work.

What I posted was just a slimmed down version of Lock(). Why would it not be necessary if Lock() does it for the "short path"?

[Joshua Liebow-Feeser]

Yeah, I agree. That was my reasoning as well.

[Dan Kortschak]

raceenabled is only true when compiled with the race detector on.

[Luke Scott]

I get that, but wouldn't TryLock, being a patch to the existing sync.Mutex, need raceAquire for the race detector?

This line:

if locked && raceenabled {

Means "if locked and compiled with race".

Wouldn't that also be important in TryLock? Perhaps I'm missing something else.

Luke

[Dan Kortschak]

On Mon, 2013-08-12 at 16:30 -0700, Luke Scott wrote:
> I get that, but wouldn't TryLock, being a patch to the existing
> sync.Mutex, need raceAquire for the race detector?

If you want to test it with the racedetector. I guess so.

> This line:
>
> if locked && raceenabled {
>
> Means "if locked and compiled with race".

> Wouldn't that also be important in TryLock? Perhaps I'm missing
> something else.

I think (guessing what's in Dmitry's head) that he thinks it's not
necessary since it's being tested in the original and it just
complicates the code you have.

[Luke Scott]

I've posted a feature request here: https://code.google.com/p/go/issues/detail?id=6123

Feel free to chime in. Looking for reasons to include:

func (m *Mutex) TryLock() bool {} // Trys to Lock(), returns true if Lock was obtained, false otherwise. Doesn't block.

At this point the name isn't important. TryLock/LockIfPossible/GetLock()/...

Personally I would want a TryLock in any situation where I would need to Lock() and TryLock() on the same mutex in different functions. Lock() would be used in a function that is expected to block, and TryLock() would be in a function that is expected not to block.

I'm not currently hurting for TryLock(), but it is in every language that provides a mutex that I could find. I feel that TryLock() should be included for completeness.

I am hurting for LockBefore(Time) though. Specifically doing something like net.Pipe where I need to set a deadline. Currently net.Pipe does not support read/write deadlines.

Luke

[Kyle Lemons]

On Mon, Aug 12, 2013 at 11:08 PM, Luke Scott <lu...@visionlaunchers.com> wrote:

I've posted a feature request here: https://code.google.com/p/go/issues/detail?id=6123

My vote is that this feature should remain unimplemented.  I have written a lot of Go, and have never felt the lack of a TryLock-like concept.  That's not for lack of having written code (mostly in Java) using it before I switched to Go, either :).

 

Feel free to chime in. Looking for reasons to include:

func (m *Mutex) TryLock() bool {} // Trys to Lock(), returns true if Lock was obtained, false otherwise. Doesn't block.

At this point the name isn't important. TryLock/LockIfPossible/GetLock()/...

Personally I would want a TryLock in any situation where I would need to Lock() and TryLock() on the same mutex in different functions. Lock() would be used in a function that is expected to block, and TryLock() would be in a function that is expected not to block.

Code is much simpler if you construct it with blocking calls and use goroutines when you need concurrency.  When you say "that is expected not to block" it sounds like you're looking for EAGAIN-like semantics, which seem to make the caller code harder to do correctly (i.e. without busy-waiting or race conditions).

 

I'm not currently hurting for TryLock(), but it is in every language that provides a mutex that I could find. I feel that TryLock() should be included for completeness.

That's not a good enough reason to support them :).  Pretty much every language with new has free, with classes has exceptions, etc.

 

I am hurting for LockBefore(Time) though. Specifically doing something like net.Pipe where I need to set a deadline. Currently net.Pipe does not support read/write deadlines.

You can easily construct your own net.Conn or io.ReadWriter pipe implementation that uses channels internally, and thus has select, and thus can have deadlines and timeouts.

 

Luke

On Sunday, August 11, 2013 8:23:35 PM UTC-7, Joshua Liebow-Feeser wrote:

Recently, I found myself wanting to be able to inspect a sync.Mutex to find out if it was locked without actually blocking. Since this isn't possible without using unsafe, I came up with a workaround:

http://godoc.org/github.com/joshlf13/sync

However, that workaround works about 2x slower than the standard sync.Mutex. What do people think of adding that functionality (in particular, two methods: LockIfPossible, which does what it sounds like, and Locked, which checks the state of the mutex - neither blocks execution)?

Cheers,
Josh

--

[Rob Pike]

I agree with kevlar. I've never wanted this in Go code and I probably use mutexes more than many on this list. And I implemented this in Plan 9 for C. But it doesn't belong here.

-rob

[Luke Scott]

On Monday, August 12, 2013 11:26:02 PM UTC-7, Kyle Lemons wrote:

On Mon, Aug 12, 2013 at 11:08 PM, Luke Scott <lu...@visionlaunchers.com> wrote:

I've posted a feature request here: https://code.google.com/p/go/issues/detail?id=6123

My vote is that this feature should remain unimplemented.  I have written a lot of Go, and have never felt the lack of a TryLock-like concept.  That's not for lack of having written code (mostly in Java) using it before I switched to Go, either :).

 

Feel free to chime in. Looking for reasons to include:

func (m *Mutex) TryLock() bool {} // Trys to Lock(), returns true if Lock was obtained, false otherwise. Doesn't block.

At this point the name isn't important. TryLock/LockIfPossible/GetLock()/...

Personally I would want a TryLock in any situation where I would need to Lock() and TryLock() on the same mutex in different functions. Lock() would be used in a function that is expected to block, and TryLock() would be in a function that is expected not to block.

Code is much simpler if you construct it with blocking calls and use goroutines when you need concurrency.  When you say "that is expected not to block" it sounds like you're looking for EAGAIN-like semantics, which seem to make the caller code harder to do correctly (i.e. without busy-waiting or race conditions).

 

I'm not currently hurting for TryLock(), but it is in every language that provides a mutex that I could find. I feel that TryLock() should be included for completeness.

That's not a good enough reason to support them :).  Pretty much every language with new has free, with classes has exceptions, etc.

 

I am hurting for LockBefore(Time) though. Specifically doing something like net.Pipe where I need to set a deadline. Currently net.Pipe does not support read/write deadlines.

You can easily construct your own net.Conn or io.ReadWriter pipe implementation that uses channels internally, and thus has select, and thus can have deadlines and timeouts.

I'm assuming that you'd use a "chan bool" as mutex for the buffer? I can't remember where I read it, but I could have sworn that a single channel used 2 mutexes internally.

So this:

if ! m.LockDelay(time.Second * 5) {

    // error

}

// do something

Would be simpler and possibly faster than:

select {

case <-lock:

    // do something

case <-time.Delay(time.Second * 5):

    // error

}

Also, how do that with a RWMutex?

[Dan Kortschak]

But what you are trying to do is get a resource, so use the chan to give
you the resource:

var tub chan []byte

func init() {
tub = make(chan []byte, 1)
tub <- make([]byte, size)
}

//...


var buf []byte

// Get buffer from tub.
select {
case buf <- tub:
case <-time.After(delay):
buf = make([]byte, size)
}

// Do stuff with buf.

// Try to put buffer back in tub.
select {
case tub <- buf:
default:

[Dan Kortschak]

On Tue, 2013-08-13 at 16:25 +0930, Dan Kortschak wrote:
> select {
> case buf <- tub:
> case <-time.After(delay):
> buf = make([]byte, size)
> }

Oops...

select {
case buf = <-tub:

[Dmitry Vyukov]

On Tue, Aug 13, 2013 at 2:15 AM, Luke Scott <lu...@visionlaunchers.com> wrote:

On Monday, August 12, 2013 2:21:12 PM UTC-7, Dmitry Vyukov wrote:

On Tue, Aug 13, 2013 at 12:13 AM, Luke Scott <lu...@visionlaunchers.com> wrote:

This is great if you're only doing a "TryLock". "Lock" and "TryLock" in different functions on the same mutex are a different story.

It should be as simple as this:

func (m *Mutex) TryLock() (locked bool) {

locked = atomic.CompareAndSwapInt32(&m.state, 0, mutexLocked)

if locked && raceenabled {

raceAcquire(unsafe.Pointer(m))

This race part is not necessary.  Users of the language implementation and the std lib are not intended to use them. It should just work.

What I posted was just a slimmed down version of Lock(). Why would it not be necessary if Lock() does it for the "short path"?

Ah, if it's a patch to std lib, then it may need some annotations.

[Joshua Liebow-Feeser]

There seems to be a general consensus that the main issue here is the tradeoff between necessity and complexity (complexity of the API, complexity of programs that would use TryLock, etc). Frankly, both strike me as relatively low.

Given that, are there any guidelines for how to deal with this tradeoff in Go? Has there been some agreement about tending towards one side or the other when in doubt?

[Kyle Lemons]

I'll add my own unsubstantiated data:

Necessity of TryLock: low

Complexity of TryLock: deceptively low

Complexity of using TryLock correctly: high

Clarity of programs using TryLock: low

Clarity of programs using channels instead: higher

[Luke Scott]

"Complexity of using TryLock correctly: high"

How so? A Lock should always be accompanied by an Unlock in the same function, typically using a defer. It would look something like this, typically:

if !m.TryLock() {

    return errors.New("Busy")

}

defer m.Unlock()

// Continue with function…

How is that complex? Now if you're doing a Lock in one function and an Unlock in another, that's where it complex. But in a majority of the cases you shouldn't be doing that. In that case you have the same problem with Lock as TryLock - TryLock is not any different than Lock other than it doesn't block.

"Clarify of programs using channels instead: higher"

Really? So this is clearer:

select {

case <-lock:

    defer func() {

        lock <-true

    }()

    // Continue with function…

default:

    return erros.New("Busy")

}

No offense, but I prefer TryLock way more in simple situations like this. I rarely use a Lock without an Unlock in the same function. TryLock would be used exactly the same way.

The TryLock is also faster because you have a single mutex. A channel is much more complex under the hood. That's why I want to have the option to go with something more primitive if I want to.

Mutexes are not "deceptive". They can be misused, but they can also be used correctly. There is no reason to punish those who plan to use it correctly. "goto" can be misused, but it's still a part of the language.

Luke

[Kyle Lemons]

On Tue, Aug 13, 2013 at 2:32 PM, Luke Scott <lu...@visionlaunchers.com> wrote:

"Complexity of using TryLock correctly: high"

How so? A Lock should always be accompanied by an Unlock in the same function, typically using a defer. It would look something like this, typically:

if !m.TryLock() {

    return errors.New("Busy")

}

defer m.Unlock()

// Continue with function…

How is that complex? Now if you're doing a Lock in one function and an Unlock in another, that's where it complex. But in a majority of the cases you shouldn't be doing that. In that case you have the same problem with Lock as TryLock - TryLock is not any different than Lock other than it doesn't block.

You're not showing the whole picture.  What does the caller do?  The caller now needs to do something like:

backoff := 100*time.Millisecond

for {

  if err := f(); err != nil {

    if err == errors.New("Busy") {

      time.Sleep(backoff)

      backoff *= 2

      if backoff > 1*time.Second {

        backoff = 1*time.Second

      }

      continue

    }

  }

  break

}

 

... and that's really complex.

"Clarify of programs using channels instead: higher"

Really? So this is clearer:

select {

case <-lock:

    defer func() {

        lock <-true

    }()

    // Continue with function…

default:

    return erros.New("Busy")

}

No offense, but I prefer TryLock way more in simple situations like this. I rarely use a Lock without an Unlock in the same function. TryLock would be used exactly the same way.

The TryLock is also faster because you have a single mutex. A channel is much more complex under the hood. That's why I want to have the option to go with something more primitive if I want to.

Of course that's not clearer.  That's still a TryLock.  The whole pattern is the problem, not the implementation.  Tell me what you want to use a TryLock to accomplish, and I'll either tell you whether I think there's a better way.

[Luke Scott]

On Aug 13, 2013, at 2:44 PM, Kyle Lemons <kev...@google.com> wrote:

On Tue, Aug 13, 2013 at 2:32 PM, Luke Scott <lu...@visionlaunchers.com> wrote:

"Complexity of using TryLock correctly: high"

How so? A Lock should always be accompanied by an Unlock in the same function, typically using a defer. It would look something like this, typically:

if !m.TryLock() {

    return errors.New("Busy")

}

defer m.Unlock()

// Continue with function…

How is that complex? Now if you're doing a Lock in one function and an Unlock in another, that's where it complex. But in a majority of the cases you shouldn't be doing that. In that case you have the same problem with Lock as TryLock - TryLock is not any different than Lock other than it doesn't block.

You're not showing the whole picture.  What does the caller do?  The caller now needs to do something like:

backoff := 100*time.Millisecond

for {

  if err := f(); err != nil {

    if err == errors.New("Busy") {

      time.Sleep(backoff)

      backoff *= 2

      if backoff > 1*time.Second {

        backoff = 1*time.Second

      }

      continue

    }

  }

  break

}

 

... and that's really complex.

That is a pure assumption, and has nothing to do with the locking implementation itself. The code I have shown deals with just the locking. If my code were anything like that I would be better off with just a regular Lock(). The whole point of TryLock is to abort if the lock cannot be established instead of block.

"Clarify of programs using channels instead: higher"

Really? So this is clearer:

select {

case <-lock:

    defer func() {

        lock <-true

    }()

    // Continue with function…

default:

    return erros.New("Busy")

}

No offense, but I prefer TryLock way more in simple situations like this. I rarely use a Lock without an Unlock in the same function. TryLock would be used exactly the same way.

The TryLock is also faster because you have a single mutex. A channel is much more complex under the hood. That's why I want to have the option to go with something more primitive if I want to.

Of course that's not clearer.  That's still a TryLock.  The whole pattern is the problem, not the implementation.  Tell me what you want to use a TryLock to accomplish, and I'll either tell you whether I think there's a better way.

Still cleaner than an actual TryLock. The pattern is still useful with certain problems. Not all problems can be solved by avoiding this pattern. If you could there would be no need for "default:".

[Kyle Lemons]

On Tue, Aug 13, 2013 at 2:56 PM, Luke Scott <lu...@visionlaunchers.com> wrote:

On Aug 13, 2013, at 2:44 PM, Kyle Lemons <kev...@google.com> wrote:

On Tue, Aug 13, 2013 at 2:32 PM, Luke Scott <lu...@visionlaunchers.com> wrote:

"Complexity of using TryLock correctly: high"

How so? A Lock should always be accompanied by an Unlock in the same function, typically using a defer. It would look something like this, typically:

if !m.TryLock() {

    return errors.New("Busy")

}

defer m.Unlock()

// Continue with function…

How is that complex? Now if you're doing a Lock in one function and an Unlock in another, that's where it complex. But in a majority of the cases you shouldn't be doing that. In that case you have the same problem with Lock as TryLock - TryLock is not any different than Lock other than it doesn't block.

You're not showing the whole picture.  What does the caller do?  The caller now needs to do something like:

backoff := 100*time.Millisecond

for {

  if err := f(); err != nil {

    if err == errors.New("Busy") {

      time.Sleep(backoff)

      backoff *= 2

      if backoff > 1*time.Second {

        backoff = 1*time.Second

      }

      continue

    }

  }

  break

}

 

... and that's really complex.

That is a pure assumption, and has nothing to do with the locking implementation itself. The code I have shown deals with just the locking. If my code were anything like that I would be better off with just a regular Lock(). The whole point of TryLock is to abort if the lock cannot be established instead of block.

Can you provide us with a concrete example, then?

"Clarify of programs using channels instead: higher"

Really? So this is clearer:

select {

case <-lock:

    defer func() {

        lock <-true

    }()

    // Continue with function…

default:

    return erros.New("Busy")

}

No offense, but I prefer TryLock way more in simple situations like this. I rarely use a Lock without an Unlock in the same function. TryLock would be used exactly the same way.

The TryLock is also faster because you have a single mutex. A channel is much more complex under the hood. That's why I want to have the option to go with something more primitive if I want to.

Of course that's not clearer.  That's still a TryLock.  The whole pattern is the problem, not the implementation.  Tell me what you want to use a TryLock to accomplish, and I'll either tell you whether I think there's a better way.

Still cleaner than an actual TryLock. The pattern is still useful with certain problems. Not all problems can be solved by avoiding this pattern. If you could there would be no need for "default:".

Using a channel as a mutex of any kind is only hiding the lock and obscuring the meaning of the code.  There are many uses for default: that have nothing to do with mutual exclusion. 

[Joshua Liebow-Feeser]

I'll show my use case, which I think is considerably cleaner with TryLock than with channels. It is this: I have a buffer that I want to use only if nobody else is using it. If it is being used, I use an algorithm that doesn't require a global buffer. It looks like this:

func DoSomething() {
    if mutex.TryLock() {
        doSomethingWithBuffer()
        mutex.Unlock()
        return
    }
    doSomethingWithoutBuffer()
}

On the other hand, if I had to use channels, I'd have:

func DoSomething() {
    select {
        case b <- bufChan:
            doSomethingWithBuffer(b)
            bufChan <- b
        default:
            doSomethingWithoutBuffer()
    }
}

Contrary to what some have said, the TryLock example doesn't strike me as any more complex than the channel example. They strike me as about the same level of complexity.

[Dan Kortschak]

On Tue, 2013-08-13 at 19:46 -0700, Joshua Liebow-Feeser wrote:
> I'll show my use case, which I think is considerably cleaner with
> TryLock than with channels.

> Contrary to what some have said, the TryLock example doesn't strike me
> as any more complex than the channel example. They strike me as about
> the same level of complexity.
>

Which is it? Cleaner or the same level of complexity? Or both?

The select is very clean.

[Joshua Liebow-Feeser]

Sorry, I changed my mind mid-way through and forgot to edit. I think they're about equally clean.

[Subramanian Ganapathy]

Hi all,

Recently, I found myself fishing for this capability. My use case was, in an earlier PR, I wrote up a code like this

```

func foo(bar *typ) bool {

    bar.mutex.RLock()

    defer bar.mutex.RLock()
   // use bar.attributes in a read only mode and compute a bool value

}
```

This was an egregious oversight and because this file was a rather long one. It took a whole to work out the root cause. One idea I had was as part of our unit testing modules while asserting the final states, I'd try acquire in shared and exclusive mode, if either fails, I'll fail the test.

I read through various arguments and i 100% support that production code shouldn't use try locks. They either use exclusive locks or mvcc style or channels. But did this testing oriented usecase come up at all? We run go test -race as part of tests too. So it appears race detector doesn't detect these problems either. Wondering if there are other ways for me to accomplish what i'm after here.

Best,

Subbu