Fix panics in streaming completions handler#60474
Conversation
| // If attribution never run, we're done. | ||
| a.attributionRun.Do(func() { | ||
| close(a.attributionFinished) | ||
| }) |
There was a problem hiding this comment.
TBH this fix feels sus. This looks like it is now possible to close a channel twice. How are you guarenteed that waitdone is only called once everything else is done?
There was a problem hiding this comment.
To elaborate further why I think this is sus. We call runAttribution in a new goroutine, which means it is possible that you call WaitDone before we have tried writing to attributionFinished? I am gonna read the code even more now, but the internal invariants here are hard to keep track of since it feels so easy to misuse.
There was a problem hiding this comment.
It should still be safe:
- One routine enters attributionRun.Do (
sync.Once) - Either:
- This piece which just closes the channel
- Or the attribution search (run async with
go) - closes it at the end, but in that case once already triggered.
There was a problem hiding this comment.
Right! so the bit I missed was attributionRun was a sync.Once. That makes this all good. Thanks!
* Fix panics in streaming completions handler * FMT (cherry picked from commit 52dffbb)
Closes #60439
2 issues:
Test plan