[protocolv2] Implement close requests #165
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
32 tasks
jackyzha0
reviewed
Jun 3, 2024
jackyzha0
approved these changes
Jun 3, 2024
jackyzha0
reviewed
Jun 3, 2024
Comment on lines
-186
to
-187
| inputWriter.write({ msg: '1', ignore: false, end: undefined }); | ||
| inputWriter.write({ msg: '2', ignore: false, end: true }); |
There was a problem hiding this comment.
this actually was helpful to test the presence of undefined fields in an object that had an optional fields but we could probably pull that test out elsewhere
There was a problem hiding this comment.
Oh, yeah I'll move it to another test so no one makes the same mistake
masad-frost
force-pushed
the
fm-fuck-promises-redux
branch
from
c2b9c0e
to
1eb0dc1
Compare
masad-frost
force-pushed
the
fm-close-request
branch
from
28bbf4c
to
5ad5ea1
Compare
This reverts commit 20e1c10.
masad-frost
added a commit
that referenced
this pull request
Jun 6, 2024
* [protocolv2] Implement close requests * Add some more checks * Add comments on for control flags * Revert "Add some more checks" This reverts commit 20e1c10.
masad-frost
added a commit
that referenced
this pull request
Jun 11, 2024
* [protocolv2] Implement close requests * Add some more checks * Add comments on for control flags * Revert "Add some more checks" This reverts commit 20e1c10.
masad-frost
added a commit
that referenced
this pull request
Jun 13, 2024
* [protocolv2] Implement close requests * Add some more checks * Add comments on for control flags * Revert "Add some more checks" This reverts commit 20e1c10.
masad-frost
added a commit
that referenced
this pull request
Jun 20, 2024
* [protocolv2] Implement close requests * Add some more checks * Add comments on for control flags * Revert "Add some more checks" This reverts commit 20e1c10.
masad-frost
added a commit
that referenced
this pull request
Jun 24, 2024
* [protocolv2] Implement close requests * Add some more checks * Add comments on for control flags * Revert "Add some more checks" This reverts commit 20e1c10.
masad-frost
added a commit
that referenced
this pull request
Jun 24, 2024
* [protocolv2] Implement close requests * Add some more checks * Add comments on for control flags * Revert "Add some more checks" This reverts commit 20e1c10.
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
We decided in protocolv2 that readers can requests writers to close (though writers might not comply). More details in Protocol.md in #127
What changed
When
reader.requestCloseis called, we send a close control message withStreamCloseRequestBit. The writer side's should then receive request and decide what to do with it.