[protocolv2] Half-close server #163
Conversation
| // Drain will make sure any read leads to an error instead of looking like | ||
| // the stream cleanly closed. | ||
| stream.inputReader.drain(); |
There was a problem hiding this comment.
We decided to switch that to not throw, and instead make the inputReader also use Result. Will do that in a follow up.
| outputWriter.write( | ||
| Err({ | ||
| code: UNCAUGHT_ERROR, | ||
| message: errorMsg, | ||
| } satisfies Static<typeof RiverUncaughtSchema>), | ||
| ); | ||
| outputWriter.close(); |
There was a problem hiding this comment.
Error handling is basically not implemented yet
Will do that in the future
| void stream.inputHandlerPromise.then((maybeDispose) => { | ||
| maybeDispose?.(); | ||
| }); |
There was a problem hiding this comment.
I don't like the way this works. I think in general cleanupStream will be phased out. Will revisit with error handling and might refactor more of the server
masad-frost
force-pushed
the
fm-half-close-server
branch
from
f7704dd
to
4553ad1
Compare
masad-frost
force-pushed
the
fm-half-close-client
branch
from
f52f04c
to
54e6719
Compare
| const streamsFromThisClient = this.clientStreams.get(message.from); | ||
| if (streamsFromThisClient) { |
There was a problem hiding this comment.
this whole code path is wrong. Will fix up in a follow as part of close request implementation, we shouldn't be immediately removing the stream as the outputWriter can still be open.
| .then(([_input, result]) => result) | ||
| .then(acceptOutput); | ||
| const [, finalize] = client.test.upload.upload({}); | ||
| void finalize().then(acceptOutput); |
There was a problem hiding this comment.
upload api is still weird to me, like most uploads dont seem like they need Init and I would expect some uniform Input and finalize would just be ending the Writable Stream and wait for some promise (so this matches the unary server return case of Rpc)
There was a problem hiding this comment.
like most uploads dont seem like they need Init
Mm, I disagree, but don't have a lot of upload examples. In my mind, most of the time you wanna target a resource and you need an init to specify it. The only upload we have in the code right now is fs.writeFile which needs to specify the file path and the size of the file.
I would expect some uniform Input and finalize would just be ending the Writable Stream and wait for some promise (so this matches the unary server return case of Rpc)
That's what the new API looks like.
There was a problem hiding this comment.
(I merged, we can half the API shape discussion separately from this PR)
masad-frost
force-pushed
the
fm-half-close-server
branch
from
4553ad1
to
d7b8479
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
Continuation for #162, server implementation
What changed
Just implementing half close for server. I'll annotate some changes in comments
Versioning