Hacker News Reader

Comment by sethvargo | Hacker News Reader

How would you fix that code?

I would write the author's example as follows:

    for ctx.Err() == nil {
        select {
        case <-ctx.Done():
            return nil
        case thing := <-thingCh:
            // Process thing...
        case <-time.After(5*time.Second):
            return errors.New("timeout")
        }
    }

The extra check for ctx.Err before the select statement easily resolves the author's issue.

It really doesn't though. It handles the case where the context might have expired or be cancelled, but there's still a race when entering the select between the ctx.Done() and reading from thingCh. You may end up processing one additional unit of work. In situations where the exit condition is channel-based, this won't work.

Additionally, this would only work if you had one predominant condition and that condition was context-based. If you have multiple ordered conditions upon which you want to exit, I can't think of how you'd express that as a range.

I'm not sure what you mean. There's always going to be a race condition between ctx.Done and thingCh, just depending on whether there's data available. This race condition is unavoidable.

I guess you're thinking of "what if thingCh and ctx.Done activate simultaneously?"

There's no real difference between happening simultaneously and happening one after another.

As for your other point, you can just write code like

    select {
    case x := <-conditionA:
        return x
    default:
    }
    select {
    case x := <-conditionB:
        return x
    default:
    }
    ...

But I've personally never needed code like this.

Right, which is noted in the post. That verbosity is, well, verbose. I generally need this in 20% of things I write.

In this use case, is it bad if the Done signal arrives the instant after you check it?

The context was introduced by the commenter. The original post does not use contexts. In general, there's a pretty common set of patterns in which multiple goroutines are writing data to different channels, and you need to ensure the data from those channels are processed with some level of priority.

Two channels is a poor way to handle priority. If data comes in on the low priority channel just before the high priority channel, you would still be blocked waiting for the low priority task to complete.

In a case like this, maybe just run two different consumer routines for the two channels, then neither would be blocked waiting on the other.

Also, this isn't semantically correct. In order to ensure that `conditionaA` is _always_ preferred over `conditionB`, you must also check if `conditionA` has received a value inside of `conditionB`:

    select {
    case a := <-conditionA:
        return a
    default:
    }
    select {
    case b := <-conditionB:
      case a := <-conditionA:
          return a
      default:

      return b
    default:
    }

It would be easier to discuss with a more concrete example. If I ever had to write code like the above I would reconsider the design and try to come up with something simpler.

I was also curious so I picked a likely-looking project on his github and indeed found an attempt to handle a channel "deterministically" at https://github.com/sethvargo/go-retry/blob/main/retry.go#L51

Honestly the whole first select seems redundant; any code that relies on this is broken as there's no other synchronization points to hang onto. You simply can't pretend the clock on the wall has anything to with the values transiting the program unless you introduce an actual synchronization point.

But OK, maybe you do have some strange performance case where this matters? In that case the whole thing could be more succinctly solved by looping on `for ctx.Err() == nil` instead of infinitely. Exactly as suggested at the start of the thread. (This would also likely be faster unless the context is under massive contention.)

It also leaks the timer until it fires if the context cancels, which seems like it would be more of a practical performance problem than any overhead to the additional select.

Context cancellation propagates (potentially) asynchronously anyway, so if you're relying on something canceling your context and that immediately appearing you already have a bug.

I've written `select { ..., default: }` enough times I also wish it had shorthand syntax - sometimes it's even clearer to range one "primary" channel and lead the code block with that check - but I cannot think of a case where relying on a deterministic select would not have led to a bug.

I think you could do which I'd argue is more idiomatic (

    for {
      if _, ok := <- doneCh; ok {
        break
      }
      select {
      case thing := <-thingCh:
        // ... long-running operation
      case <-time.After(5*time.Second):
        return fmt.Errorf("timeout")
      }
    }

Which goes along w/ https://github.com/golang/go/wiki/CodeReviewComments#indent-... of "Indent error flow".

edit: nvm, your break would be blocked until one of the other channels produced a value. you'd need to check for the doneCh redundantly again in the select.

Reply on news.ycombinator.com