RFC: take 2 on streams #249
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
masad-frost
requested review from
jackyzha0 and
Monkatraz
and removed request for
a team
jackyzha0
approved these changes
Jul 31, 2024
| @@ -72,8 +73,7 @@ type UploadFn< | |||
| > = ( | |||
| reqInit: ProcInit<Router, ProcName>, | |||
| options?: CallOptions, | |||
| ) => { | |||
| reqWriter: WriteStream<ProcInput<Router, ProcName>>; | |||
| ) => Writable<ProcInput<Router, ProcName>> & { | |||
There was a problem hiding this comment.
rename Router -> Service as a more accurate description
There was a problem hiding this comment.
will fix in another PR when fixing the client.
masad-frost
commented
Jul 31, 2024
masad-frost
commented
Jul 31, 2024
Monkatraz
reviewed
Jul 31, 2024
router/procedures.ts
Outdated
| ): Promise<Result<Static<ResponseData>, Static<ResponseErr>>>; | ||
| } | ||
|
|
||
| export class UploadProcedureHandlerParam< |
There was a problem hiding this comment.
I feel like the name of these classes is non-intuitive despite their verbosity - I feel like they could have plain names, like in this case something like UploadStream or HandlerUploadStream
There was a problem hiding this comment.
How about UploadReadable? I'm trying to avoid using the term stream tbh, it's quite overloaded
|
As much as I hate to merge into protocolv2 and leaving the branch non-functional/broken, I'm gonna go ahead an merge this to keep my PRs small and build on top of these abstractions without dealing with stacked branches too much. |
This was referenced Aug 1, 2024
masad-frost
added a commit
that referenced
this pull request
Aug 1, 2024
## Why Follow up on #249 to actually use the new interfaces. Removed extra refactor done in #249 that merges the interfaces as that proved to be a challenging API (un-yak-shave 🙅 🐃) ## What changed - Removed close requests (mostly cherry-picked from #248) - Otherwise a simple swapping out of the interfaces ## Versioning - [ ] Breaking protocol change - [ ] Breaking ts/js API change <!-- Kind reminder to add tests and updated documentation if needed -->
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.
Got some comments on streams being complicated, so this is a take on simplified API.
streamcall would have both read and write capabilities on the same object)