Go: proposal: add sync.Mutex.TryLock

[deanveloper]

This has been brought up before (#6123)

It seems the ultimate reason this was rejected was because there wasn't a good use case for it, which is what I have now, which is a progress bar for a parallel process.

Essentially, I was loading in several extremely large files, and I wanted a progress bar for how close I was to finishing. The progress bar could make very good use of TryLock.

Note that a server is loading in these files, and the client wants to view, in realtime, the progress of the changes.

Here is how I would like to implement a ProgressBar's Add function:

// adds n to the current progress, and tells clients. is thread-safe.
func (b *ProgressBar) Add(n int) {
    atomic.AddInt64(&b.Progress, int64(n))

    if b.Progress >= b.Max {
        b.once.Do(b.updateClientsDone)
        return
    }

    if b.pctMx.TryLock() {
        defer b.pctMx.Unlock()
        b.updateClients()
    }
}

This is intended to work within a worker pool and be called (possibly) millions of times, so I really don't want to have it wait on any mutexes for long, or create any separate goroutines inside of it.

It also needs to be in some kind of mutex, as if it gets ran out of order, then the progress bar may begin to get jittery (as old values may get sent after new ones)

I also do not want to send the clients _every_ update, but I do want to send updates as often as possible without significantly slowing down the program by _waiting_ on the b.pctMx.

I guess using a 3rd party library would work, but I'd really expect this to be first-class support, especially because this is a feature in most languages, and Go in particular has very good support for concurrency.

Essentially I guess what this proposal is trying to say, is that TryLock is the best way to run non-essential code inside a function that is called extremely often. If there is a better way to do this, please let me know.

[andybons]

It also needs to be in a mutex, as if it gets ran out of order, then

It seems this got cut off

I guess using a 3rd party library would work, but I'd really expect this to be first-class support, especially because this is a feature in most languages, and Go in particular has very good support for concurrency.

Why not implement (or use an existing) third-party library that provides this, then you can see how many other gophers find it useful? That will provide a more compelling argument for adding it.

[bcmills]

If you use a 1-buffered channel as a mutex, TryLock is just a select with a default case.

[bcmills]

That said, it would be helpful to see the broader context: the atomic implies that you'll have cache contention either way, so it may be simplest — and equally or nearly equally scalable — to use a mutex (or channel) unconditionally on every Add.

[bcmills]

(In particular, it would be helpful to know what updateClients and updateClientsDone are actually doing. You appear to be communicating the progress changes by sharing memory — the b.Progress field — but the Go style in general is to share by communicating.)

[deanveloper]

It seems this got cut off

Fixed

That said, it would be helpful to see the broader context: the atomic implies that you'll have cache contention either way, so it may be simplest — and equally or nearly equally scalable — to use a mutex (or channel) unconditionally on every Add.

No, because b.updateClients() performs a socket connection, I do not want other goroutines waiting on the socket connection

(In particular, it would be helpful to know what updateClients and updateClientsDone are actually doing. You appear to be communicating the progress changes by sharing memory — the b.Progress field — but the Go style in general is to share by communicating.)

I am communicating progress by sending it over a socket connection. It needs to be in a mutex, or else the progress bar gets pretty jittery from the delay of context-switches in between evaluating the current progress and sending it over the socket.

| gorout 1 | gorout 2 |
| ------------- | ------------- |
| load file | |
| evaluate progress (58%) | |
| | load file |
| | evaluate progress (60%) |
| | send 60% over socket |
| send 58% over socket | |

note that evaluating and sending progress is not super essential, but it does take a long time, and it needs to be done mutually exclusively (as illustrated above). It also should not be done in a _raw_ mutex because it is non-essential - so I do not want goroutine 2 to need wait for goroutine 1. This would slow down all of my workers. I'd much rather just have the progress be sent again later.

[deanveloper]

Why not implement (or use an existing) third-party library that provides this, then you can see how many other gophers find it useful? That will provide a more compelling argument for adding it.

I tried to do this, it seems like TryLock seems to get pretty badly misused... People apparently tend to use it to busy-wait, which is just a bad practice and shouldn't be what TryLock is meant to do. I really am starting to think that it should just left to a third-party library

[DeedleFake]

I don't know your particular use case or your code, but what about something like this:

1. Allocate a channel with a buffer of one, as suggested by @bcmills.
2. Spin up a third, dedicated worker that listens on that channel and then sends progress updates to the clients. I don't know how your ProgressBar type is instantiated, but it should probably be started at that point, if possible.
3. In your above example, replace 'send over socket' with a send to that channel in a select with a default case, thus preventing blocking.

I do have a question, though: You mention that it gets 'jittery'. Do you mean that it jumps back and forth or just that it pauses and then jumps suddenly? What exactly are you trying to prevent from happening?

[bcmills]

Based on your description, I would expect to see something like:

// Add adds n to the current progress.
func (b *ProgressBar) Add(n int) {
    st := <-b.state
    st.progress += n
    for c, threshold := range st.waiting {
        if st.progress > threshold {
            c <- st.progress
            delete(st.waiting, c)
        }
    }
    b.state <- st
}

func (b *ProgressBar) Await(threshold int) (progress int) {
    st := <-b.state
    if st.progress > threshold {
        b.state <- st
        return st.progress
    }

    c := make(chan int, 1)
    st.waiting[c] = threshold
    b.state <- st

    return <-c
}

Then the “progress” goroutine (per @DeedleFake's suggestion) would look something like:

progress := 0
for progress < b.Max {
    progress = b.Await(progress)
    updateClients(progress)
}

[DeedleFake]

I was actually thinking something more like this:

func (b *ProgressBar) Add(n int) {
  progress := atomic.AddInt64(&b.Progress, int64(n))

  select {
  case b.updateChan <- progress:
  default:
  }
}

func (b *ProgressBar) updater(ctx context.Context) {
  for {
    select {
    case <-ctx.Done():
      return

    case progress := <-b.updateChan:
      if progress >= b.Max {
        b.once.Do(b.updateClientsDone)
        return
      }

      b.updateClients(progress)
  }
}

You don't need to ever close or drain b.updateChan. It's fine to just leave it with data in its buffer; the garbage collector will clean it up when ProgressBar gets freed. Just make sure that the context gets canceled or it won't get freed, since updater() will still have a reference to the ProgressBar.

It sounds like ProgressBar is an implementation of a network client for updating a remote progress bar, so I assume that updateClients() and updateClientsDone() are sending the information about the current progress to the remote clients.

[deanveloper]

https://github.com/golang/go/issues/27544#issuecomment-419515859

Yeah this would actually work pretty well, I like this. (I personally don't like the context construct and hope it either gets replaced with something better or something more clear like a Canceller or something).

Also - since there's no guarantee that the last progress update will be sent over the channel (because of the default case) it must be handled in the Add function, not the updater. But other than that, yeah this is a pretty good solution for this use case and similar ones.

For other use cases, I guess you could just use a CAS on an integer to 1 for locked and 0 for unlocked. I'll close this issue since I think TryLock will encourage bad practices, and there are other ways to solve the same problem already.

[DeedleFake]

Glad I could help.

The context is just being used for cancellation. If you don't want to use the whole context system just for that, you can easily replicate the functionality with a chan struct{} and another sync.Once that closes the channel.

[andybons]

Thank you all. This thread is a great example of helpful suggestions, keeping an open mind, and excellent discourse leading to a solution everyone seems happy with.