Arvados

14458-controller-panic @ 925f7e4c35fb335b3f57000371592db9f2206766

With a buffered errorChan, it seems like wg.Done() and cancelFunc() can be called after the error is sent but before <-errorChan wins the select race, which means <-sharedContext.Done() can win the select race while the errors slice is empty, even though there is in fact an error to report.

How about making errorChan:=make(chan error, len(h.handler.Cluster.RemoteClusters)) and, when Done(), collecting errors in a for len(errorChan)>0 loop? Seems like we only care about the errors if we get to the "give up" case, so in the meantime accumulating them in a channel seems more convenient than accumulating them in a slice. #14262#note-12

Tom Clegg wrote:

With a buffered errorChan, it seems like wg.Done() and cancelFunc() can be called after the error is sent but before <-errorChan wins the select race, which means <-sharedContext.Done() can win the select race while the errors slice is empty, even though there is in fact an error to report.

Here's my reasoning:

1. wg.Wait() unblocks to call cancelFunc() only after all goroutines have completed.
2. At that point, every goroutine that is going to produce an error will have already put its error in the buffered channel
3. In the loop, the receive on the error channel has priority over the cancellation channel
4. Therefore, the error channel will be drained before matching the <-sharedContext.Done() case

... so on further reading, I see that the select statement in Go don't actually have priority between channels, so it won't work like that. Argh!

(So the real reason I originally wrote this code using mutexes and shared state is that it was frankly easier for me to reason about than Go channels and all their edge cases)

You're right, it is simpler to keep them in the buffered channel and drain it when it is done.

Now 14458-controller-panic @ 6938e8cff1632d597cfbd333e4d5176805b628c6

With for range errChan, the behavior changes a bit: if the parent context cancels, the handler blocks until all of the remote-req goroutines notice that and return. (Previously, and still in the success case, the handler returns immediately, letting the remote-req goroutines run until they notice the cancelled context.) Is this deliberate? I suggested "for len(errChan)>0" because I thought the "return immediately, let goroutines run" strategy seemed sensible to use across the board -- e.g., the "request finished" log message would happen right away.

LGTM

Tom Clegg wrote:

With for range errChan, the behavior changes a bit: if the parent context cancels, the handler blocks until all of the remote-req goroutines notice that and return. (Previously, and still in the success case, the handler returns immediately, letting the remote-req goroutines run until they notice the cancelled context.) Is this deliberate? I suggested "for len(errChan)>0" because I thought the "return immediately, let goroutines run" strategy seemed sensible to use across the board -- e.g., the "request finished" log message would happen right away.

No, that wasn't deliberate, I wasn't thinking about the case of the parent context being cancelled. I see what you mean, draining the buffer but not waiting for it to close would allow it to return slightly earlier if there are outstanding requests.

LGTM

Changed range errorChan to len(errorChan) > 0 and merged.