simplify transport. #296
Merged
simplify transport. #296
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A substantial amount of the complexity of the transport package is due the interface: we pass contexts to functions that do IO, and then jump through hoops to respect those contexts. Instead, this patch changes Transport and Codec's Close() to work like net.Close(): it interrupts any outstanding IO. We demand the same semantics from the io.ReadWriteCloser that we pass in, which seems like not a tall order. Still TODO: - [ ] rework the rpc subsystem's shutdown logic. The basic idea is we spawn an extra goroutine to wait on context.Done() and then call close, but the shutdown logic is intricate and I am too tired right now; this patch introduces test regressions for now. - [ ] Tweak the docs on how this changes the interface for e.g. NewConn
zenhack
added a commit
to zenhack/go-capnp
that referenced
this pull request
Nov 28, 2022
This is a self-contained enough change that it seemed worth submitting independently of capnproto#296; this moves the actual IO calls on the transport into a new pair of goroutines (one for send, one for receive), so that the receive loop and the send goroutine can always be responsive to context cancels. This will become important when we no longer require the IO operations themselves to respond to a context.
zenhack
added a commit
to zenhack/go-capnp
that referenced
this pull request
Nov 28, 2022
This is a self-contained enough change that it seemed worth submitting independently of capnproto#296; this moves the actual RecvMessage() call on the transport into a new goroutines (one for send, one for receive), so that the receive loop always be responsive to context cancels. This will become important when we no longer require the IO operations themselves to respond to a context. send() can stay as-is; it would be abnormal for write to block indefinitely, so we can rely on that one to return in a reasonable amount of time. Meanwhile, the new reader goroutine is safe to leave running until late into the shutdown logic, since the shutdown logic doesn't itself want to read from the transport, and any messages read won't be acted on since the receive loop has shut down.
- Add a timeout during shutdown for aborts - Make sure we are actually reading from the channel
|
This is mostly working, but there are some intermittent test failures I don't fully understand yet. @lthibault, I think it's worth a look, though we shouldn't hit merge until we've nailed down those issues. As far as performance data; before: After: |
lthibault
previously approved these changes
Nov 28, 2022
Presumably this couldn't happen before we added the timeout during shutdown, but now I am seeing an occasional panic here without this if.
This fixes some occasional test failures where questions were sent even though the context is canceled before the relevant functions were called; these are dependent on sendMessage to check the context before actually doing the send.
|
@lthibault, ok, I think this is ready. |
Merged
lthibault
approved these changes
Nov 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A substantial amount of the complexity of the transport package is due
the interface: we pass contexts to functions that do IO, and then jump
through hoops to respect those contexts. Instead, this patch changes
Transport and Codec's Close() to work like net.Close(): it interrupts
any outstanding IO. We demand the same semantics from the
io.ReadWriteCloser that we pass in, which seems like not a tall order.
Still TODO:
spawn an extra goroutine to wait on context.Done() and then call close,
but the shutdown logic is intricate and I am too tired right now; this
patch introduces test regressions for now.
Marking this as a draft for now, until I find time to tie up the loose ends.