[protocolv2] Add abort support, move protocol errors to abort status (docs only) #175
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
commented
Jun 6, 2024
Comment on lines
147
to
+149
| - The `StreamClosedBit` (`0b00100`) MUST be set for the last message of a stream. | ||
| - The `StreamCloseRequestBit` (`0b10000`) MUST be set for a message that is requesting the other side to close the stream. | ||
| - Bits `0b01000` is reserved for future use and is currently unused. | ||
| - The `StreamAbortBit` (`0b01000`) MUST be set when a stream is to be abruptly closed due to cancellations or an internal error condition. |
There was a problem hiding this comment.
I'm considering swapping the bits for StreamAbortBit and StreamClosedBit. The old implementation of StreamClosedBit is actually closer to an abort, though the don't carry the right payload (ControlClose instead of ProtocolError), the implementations don't seem to peek the payload anyway just the control flag.
jackyzha0
approved these changes
Jun 6, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 6, 2024
32 tasks
masad-frost
added a commit
that referenced
this pull request
Jun 6, 2024
Old stream closures are more akin to what abort is supposed to do so it's better to swap them to maintain better compatibility #175 (comment)
masad-frost
added a commit
that referenced
this pull request
Jun 11, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 11, 2024
Old stream closures are more akin to what abort is supposed to do so it's better to swap them to maintain better compatibility #175 (comment)
masad-frost
added a commit
that referenced
this pull request
Jun 13, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 13, 2024
Old stream closures are more akin to what abort is supposed to do so it's better to swap them to maintain better compatibility #175 (comment)
masad-frost
added a commit
that referenced
this pull request
Jun 20, 2024
masad-frost
added a commit
that referenced
this pull request
Jun 20, 2024
Old stream closures are more akin to what abort is supposed to do so it's better to swap them to maintain better compatibility #175 (comment)
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
Old stream closures are more akin to what abort is supposed to do so it's better to swap them to maintain better compatibility #175 (comment)
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
Old stream closures are more akin to what abort is supposed to do so it's better to swap them to maintain better compatibility #175 (comment)
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 <j.zhao2k19@gmail.com>
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
StreamClosedBitshould be reserved for normal closures where the writer decides to close gracefully, overloading it to transmitProtocolErrors which lead to abrupt closure can be confusing and problematic. Since we plan on adding abort/cancellation support, might as well go ahead and do this now while we're rolling out a breaking changeWhat changed
Added
StreamAbortBitand moved protocol errors there. This also involves us adding abort support instead of clean closes.Versioning