Safe StreamEvents write loop #14557
Safe StreamEvents write loop #14557
Conversation
kasey
force-pushed
the
safe-stream-write-loop
branch
from
d69752c to
4e9d21a
Compare
kasey
force-pushed
the
safe-stream-write-loop
branch
from
147fc08 to
338ffba
Compare
| go es.outboxWriteLoop(ctx, cancel, sw) | ||
| if err := es.recvEventLoop(ctx, cancel, topics, s); err != nil { | ||
| log.WithError(err).Debug("Shutting down StreamEvents handler.") | ||
| } | ||
| cleanupStart := time.Now() | ||
| es.waitForCleanup() |
There was a problem hiding this comment.
for other reviewers, this is key to fixing the panic here. previously StreamEvents would exit and the outboxWriteLoop could still be writing to a writer that was cleaned up via regular HTTP processes. now there is this wait and inside the writes wrapped with deadlines
| defer close(es.outbox) | ||
| defer func() { | ||
| cancel() | ||
| }() |
There was a problem hiding this comment.
I think you need es.exit() here. Otherwise this function will return with an error, the Shutting down StreamEvents handler log will be printed and the handler will be stuck on es.waitForExit() indefinitely because outboxWriteLoop will just keep looping. On the other hand, I am not sure if there aren't negative consequences of exiting here instead of in outboxWriteLoop.
There was a problem hiding this comment.
as discussed offline the context cancellation in recvEventLoop causes outboxWriteLoop to break out of the loop and call exit on its own.
|
Well, deep source is stuck, so I'm going to fix it's one complaint and push to try to start it again. |
kasey
force-pushed
the
safe-stream-write-loop
branch
from
80027ea to
168b26a
Compare
| @@ -0,0 +1,90 @@ | |||
| package util | |||
There was a problem hiding this comment.
Nice additions to this pkg!
kasey
dismissed stale reviews from prestonvanloon and james-prysm
via
bf8a6c8
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
StreamEvents handler now waits for the write loop to exit. In order to do this safely it uses the ResponseController type and calls SetWriteTimeout before each write (otherwise the write goroutine could get stuck blocked on the reader and wedge the http handler). Doing all this required some additional work on testing setup and support, in particular watching logs for errors since these are otherwise hard to detect with a streaming http response.
Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements