[protocolv2] Half-close client #162
Merged
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
|
Test will fail miserably as I deferred fixing them to the server implementation |
masad-frost
force-pushed
the
fm-half-close-client
branch
from
54f5a03
to
f52f04c
Compare
2 tasks
masad-frost
force-pushed
the
fm-half-close-client
branch
from
f52f04c
to
54e6719
Compare
32 tasks
jackyzha0
approved these changes
Jun 3, 2024
| import { Value } from '@sinclair/typebox/value'; | ||
| import { PayloadType, ValidProcType } from './procedures'; | ||
|
|
||
| type RPCFn< |
There was a problem hiding this comment.
Suggested change
| type RPCFn< | |
| type RpcFn< |
rust brain moment
There was a problem hiding this comment.
HTML or Html, URL or Url? 🤷
lol i'll change it on the base branch
| } | ||
|
|
||
| if (!Value.Check(ControlMessageCloseSchema, msg.payload)) { | ||
| if (Value.Check(AnyResultSchema, msg.payload)) { |
There was a problem hiding this comment.
completely unrelated to this pr but could be cool to do compiled versions of common schema checks:
https://github.com/sinclairzx81/typebox#typecheck-typecompiler
| infer __UploadInputMessage, | ||
| infer UploadOutputMessage, | ||
| (...args: never) => Promise<infer UploadOutputMessage>, |
| : never | ||
| : Procedure extends object & { stream: infer StreamHandler extends Fn } | ||
| ? Awaited<ReturnType<StreamHandler>> extends [ | ||
| ? ReturnType<StreamHandler> extends [ |
There was a problem hiding this comment.
still weird that some procedure constructors are async and others aren't but maybe thats fine 🤷
There was a problem hiding this comment.
It's just Rpc that returns a promise
masad-frost
added a commit
that referenced
this pull request
Jun 6, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 11, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 13, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 20, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 24, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 24, 2024
masad-frost
added a commit
that referenced
this pull request
Aug 16, 2024
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 <[email protected]>
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.
Why
Starting to implement half-close semantics, this targets the client side. Please read Protocol.md in #127 to understand what's going on.
What changed
streamreturns an[inputWriter, outputReader]tupleuploadreturns an[inputWriter, finalizerFn]tuple, wherefinalizerFnreturnsPromise<OutputResult>subscriptionreturnsoutputReaderrpcreturnsPromise<OutputResult>createProcStreamworks on the server), and then construct the return interface.rpcsendsCloseBitandOpenBitas part ofinitis an optional thing, and would work for other types of procs if we want. In theory, withrpcyou can send init with anOpenBitonly and then send aCloseControlmessage as a follow. In a more extreme world, yourstreamcan sendCloseBitandOpenBitas part ofinitwhich instantly closes theinput(client-to-server) stream. We might want to updateProtocol.mdto indicate that all types of procedures work the same way, and things likerpcsending open and close in one message is just an optimization.CloseBitwithinit, since we don't need theinputstream.