[protocolv2] Implement server abort #200
Conversation
masad-frost
requested review from
Monkatraz and
jackyzha0
and removed request for
a team
masad-frost
force-pushed
the
fm-abort-server-side
branch
from
5b59cec
to
69dc984
Compare
| // TODO handle never resolving after cleanup/full close | ||
| // which would lead to us holding on to the closure forever |
There was a problem hiding this comment.
Can you even avoid the memory leak if the handler never resolves? You could have a timeout condition (where the timeout only starts after everything is closed) but the handler would still have a reference to a lot of things in the context which may force it to stick around. Still would be better than stalling forever though
There was a problem hiding this comment.
we just need to make sure we avoid capturing things that are not needed in a closure, but i think there's no way about it. I think we should just have a log "procedure X didn't exit after Y seconds of stream end" or something.
| * Aborts are not the same as closing streams gracefully, please refer to | ||
| * the river documentation to understand the difference between the two concepts. | ||
| */ | ||
| abortController: AbortController; |
There was a problem hiding this comment.
I wonder if making this a function named something like abortProcedureUngracefully would encourage the right practices here (not really a throw, though)
There was a problem hiding this comment.
I thought about using a function here, but i wanted to give people a way to track their own aborts, so that they can go abortController.signal. I think we'll have to see some people use this stuff and iterate
| ]); | ||
| expect(clientOutputReader.isClosed()); | ||
| expect(clientInputWriter.isClosed()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
we can probably simplify this test code into only the e2e path
| * handshake metadata with some auth mechanism to identify the | ||
| * the request sender. | ||
| */ | ||
| session: Session<Connection>; // TODO: only expose a subset interface of session |
There was a problem hiding this comment.
we can probably also just use Omit<Session<Connection>, ...> here so we do it at the type level, idt it matters if we actually strip stuff
masad-frost
force-pushed
the
fm-abort-client-side
branch
from
f3a4134
to
9326512
Compare
masad-frost
force-pushed
the
fm-abort-server-side
branch
from
69dc984
to
c50ba87
Compare
All the changes are documented in `Protocol.md` but here's a summary: - Handle invalid client requests by sending a close with an error back - This was the main motivation for the change. While we could sort-of implement this error response without the other changes, things are setup in such a way where it is very hard to implement correctly without deeper changes in how we handle closing. - Add more robust closing mechanics - Half-close states - Close signals from read end of the pipes - Abort full-closure (for errors and cancellation) - Switch from `Pushable` and `AsyncIterator` APIs to a `ReadStream` and `WriteStream` - All procedures have `init` and some have `input` While the changes are not strictly backwards compatible, hence the major protocol bump, the system can still operate across versions to some extent. See PRs linked below for more information on the above # TODOs - [x] Define protocol and update doc #111 - [x] Design stream abstractions #118 - [x] Redsigned in #249 - [x] Implement stream abstractions - [x] ReadStream #130 - [x] WriteStream #132 - [x] All streams have init, some have input. - [x] Protocol change documented in #153 - [x] Implementation change #159 - [x] Use stream abstractions & implement protocol closing semantics - [x] Protocol: Implement close requests from readers #165 - [x] Protocol: Implement half-close - [x] Client #162 - [x] Server #163 - [x] Simple s/Pushable/Stream replacement - [x] Client #136 - [x] Server #137 - [x] Make `Input` iterator on the server use `Result` so we can signal stream closes, client disconnects, and aborts #172 - [x] Add Abort mechanism - [x] Docs update #175 - [x] Implement abort - [x] Client #193 - [x] Server #200 - [x] Add `INVALID_REQUEST` to schema #107 - [x] Handle/send back `INVALID_REQUEST` errors with an abort bit #203 - [x] Handle/send back `INTERNAL_RIVER_ERROR` with an abort bit #203 - [x] Send abort bit with `UNCAUGHT_ERROR` #201 - [x] Abort tombstones #204 - [ ] Try to find uncovered areas to test - [ ] `undefined` value for `init`, `input`, & `output`. - [ ] Update docs - [ ] Changelog --------- Co-authored-by: Jacky Zhao <j.zhao2k19@gmail.com>
Why
Server-side implementation for #175
What changed
For tests, recommend disabling whitespace diff as i nested tests in more describe blocks
Mostly abort implementation which is symmetrical with the client implementation. The handler context has mechanisms to deal with abort.
ServiceContextWithTransportInfois nowProcedureHandlerContext.fromandtofrom context, they can be retrieved fromsessionstreamIdfrom context as it's an internal thingabortControllerto context, this is the controller for which the handler can decide to abort the requestclientAbortSignalto context, this is a signal that is triggered when the client sends an abort, or when the client disconnects.I'm honestly not super happy with the interface, i think it can be confusing to have a controller and a signal that are separate, ideas welcome. One idea I had is to pass an
abortfunction to the context, and an abort signal that is triggered when any of happens:with the abort reason indicating what happened. Updating the interface should be easy, but this PR gets us to a place where we can iterate on the interface.