/cc @adg @Sajmani

I would argue that (2) is correct. There is no reason to call Err without Done: if nothing else, it encourages the writing of racy code.

(To poll whether a Context is done without blocking, you can use a select with a default case.)

It's been a while, but if I recall, the reason to call Err might be something like:

for ctx.Err() != nil {
  // do short operation that can't easily select on Done()
}

Otherwise this would need to look like

outer: for {
  select {
    case <-ctx.Done():
      break outer
    default:
      // do short operation 
  }
}

I don't recall the exact specifics of why this came up, though. I don't see why expecting to be able to call Err() without Done() is inherently racy though? I suppose if you were checking Err() and then doing something with the context, that could be racy (sort-of... the context itself would have to be racy), but I don't see why that would be NOT the case in the event that you do something in the default clause of the select with the context object either:

outer: for {
  select {
    case <-ctx.Done():
      break outer
    default:
      // context could actually be Done here anyway
  }
}

A more concrete use-case would be helpful. In my experience, more often than not the loops you're describing actually look like:

for {
  select {
  case <-ctx.Done():
    return nil, ctx.Err()
  default:
  }
  // Do short operation that can't easily select on Done().
}
return someValue, nil

Calling Err without waiting on Done is racy if the particular implementation of the Context interface implements the Err method without adding synchronization beyond closing the Done channel. There is currently no prohibition on such an implementation. (The cancelCtx in the standard library uses a sync.Mutex, but that's more to guard the fast-path cancelers than c.err per se.)

The fundamental problem is this: if the implementation of Err doesn't add a hard sequence point to the program, then the compiler (or the CPU, for that matter!) can assume that its value never changes and optimize your loop to:

for {
  // do short operation that can't easily select on Done()
}

But if Err does add a hard sequence point, it becomes much more expensive: for example, cancelCtx.Err acquires a Mutex lock, which means that if you have N CPUs all executing those loops simultaneously, your code will appear to execute many copies of the short operation in parallel but actually spend most of its time moving the mutex around from cache to cache. (It's a similar problem to the one underlying #17973.)

I guess that's why I was hoping the documentation could be clarified 😄. It sounds like the documentation should say something along the lines of "It is undefined behavior to call Err() before blocking on Done()."

(I was hoping that it would be reasonable to give Err() a meaning before Done() in order to make it safe, but it sounds like documenting this behavior is insufficient to make it reliable since other implementations of Context presumably don't couple these two.)

(sorry for all the updates)

Would it make sense to have govet check for this misuse of Err?

I believe this issue can be closed now that #19856 was accepted and implemented.

Duplicate of #19856