[sethvargo]

How would you fix that code?

[assbuttbuttass]

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.

[sethvargo]

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.

[schrodinger]

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.