[protocol v2] RFC PROTOCOL V2 #111

masad-frost · 2024-05-09T19:55:08Z

Why

Currently half-closes initiated by the server are not documented and sort of undefined behavior in the implementation. This is critical to adding INVALID_REQUEST errors.

What changed

Document Error type more explicitly
Document Result type more explicitly
Document Control type more explicitly
Propose protocol errors
Proposed changes for server initiated closes
- These can either be normal closure or a protocol error closure
Document reader and writer semantics

The changes should be somewhat backwards compatible, existing servers should be able to serve new clients with the exception of server initiated closes/protocol errors, I don't anticipate us needing to do a crazy migration to handle these changes.

Versioning

Breaking protocol change
Breaking ts/js API change

PROTOCOL.md

Co-authored-by: Jacky Zhao <j.zhao2k19@gmail.com>

PROTOCOL.md

masad-frost · 2024-05-10T01:51:08Z

@jackyzha0 @lhchavez I updated the document and the PR description. We now allow half-closes, mostly convinced by the fact that uploads are kinda wonky without 2 way close, adding another control message or flag for that didn't feel right.

masad-frost · 2024-05-11T01:37:23Z

PROTOCOL.md

+
+`Control` is a payload that is wrapped with `TransportMessage`.
+
+??? TODO Faris: there's no differentiation between a `Control` in the protocol and an `Input` message with the the same schema. Yikes. Should probably fix this in this protocol bump.


Resolved on slack

Suggested change

??? TODO Faris: there's no differentiation between a `Control` in the protocol and an `Input` message with the the same schema. Yikes. Should probably fix this in this protocol bump.

* Update PROTOCOL.md * Update PROTOCOL.md Co-authored-by: Jacky Zhao <j.zhao2k19@gmail.com> * Update PROTOCOL.md * half-close semantics * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * fmt * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Update PROTOCOL.md * Include reader-writer semantics --------- Co-authored-by: Jacky Zhao <j.zhao2k19@gmail.com>

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>

Update PROTOCOL.md

31bb671

masad-frost requested a review from a team as a code owner May 9, 2024 19:55

masad-frost requested review from bradymadden97 and removed request for a team May 9, 2024 19:55

jackyzha0 reviewed May 9, 2024

View reviewed changes

lhchavez reviewed May 9, 2024

View reviewed changes

PROTOCOL.md Outdated Show resolved Hide resolved

PROTOCOL.md Outdated Show resolved Hide resolved

PROTOCOL.md Outdated Show resolved Hide resolved

PROTOCOL.md Outdated Show resolved Hide resolved

masad-frost and others added 3 commits May 9, 2024 14:43

Update PROTOCOL.md

0110a38

Co-authored-by: Jacky Zhao <j.zhao2k19@gmail.com>

Update PROTOCOL.md

4efa567

half-close semantics

f023fd3

masad-frost commented May 10, 2024

View reviewed changes

PROTOCOL.md Outdated Show resolved Hide resolved

masad-frost added 16 commits May 9, 2024 18:25

Update PROTOCOL.md

b0c7187

Update PROTOCOL.md

8bbbb07

Update PROTOCOL.md

d24d06b

Update PROTOCOL.md

b656bd3

Update PROTOCOL.md

cf5e3cf

Update PROTOCOL.md

8c9e29c

Update PROTOCOL.md

6035f91

Update PROTOCOL.md

36a0d52

Update PROTOCOL.md

96a1682

Update PROTOCOL.md

ed2f250

Update PROTOCOL.md

a174575

Update PROTOCOL.md

1be23ea

Update PROTOCOL.md

ef4a0d4

Update PROTOCOL.md

c99616c

Update PROTOCOL.md

64be3d5

fmt

d7829f1

masad-frost changed the title ~~RFC PROTOCOL V2 (with errors and one way close)~~ RFC PROTOCOL V2 (with errors and server initiated closes) May 10, 2024

masad-frost added 2 commits May 9, 2024 20:20

Update PROTOCOL.md

2625da7

Update PROTOCOL.md

4c7f6eb

Update PROTOCOL.md

b9ee6a3

masad-frost changed the base branch from main to protocolv2 May 11, 2024 01:34

Merge branch 'protocolv2' into fm-protocol-v2

c6d6ae6

masad-frost commented May 11, 2024

View reviewed changes

masad-frost added 2 commits May 10, 2024 18:37

Update PROTOCOL.md

9688f3d

Update PROTOCOL.md

71a33fc

masad-frost changed the title ~~RFC PROTOCOL V2 (with errors and server initiated closes)~~ [protocol v2] RFC PROTOCOL V2 (with errors and server initiated closes) May 11, 2024

This was referenced May 11, 2024

[protocol v2] RFC: custom streams #118

Merged

RFC: handle bad messages #98

Closed

Base branch for protocol v2 #127

Merged

Include reader-writer semantics

1ed4b40

masad-frost changed the title ~~[protocol v2] RFC PROTOCOL V2 (with errors and server initiated closes)~~ [protocol v2] RFC PROTOCOL V2 May 16, 2024

masad-frost merged commit f86f5a9 into protocolv2 May 17, 2024
3 checks passed

masad-frost deleted the fm-protocol-v2 branch May 17, 2024 23:55

This was referenced May 19, 2024

ReadStream implementation #130

Merged

WriteStream implementation #132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[protocol v2] RFC PROTOCOL V2 #111

[protocol v2] RFC PROTOCOL V2 #111

masad-frost commented May 9, 2024 •

edited

Loading

masad-frost commented May 10, 2024 •

edited

Loading

masad-frost May 11, 2024


		`Control` is a payload that is wrapped with `TransportMessage`.

		??? TODO Faris: there's no differentiation between a `Control` in the protocol and an `Input` message with the the same schema. Yikes. Should probably fix this in this protocol bump.

[protocol v2] RFC PROTOCOL V2 #111

[protocol v2] RFC PROTOCOL V2 #111

Conversation

masad-frost commented May 9, 2024 • edited Loading

Why

What changed

Versioning

masad-frost commented May 10, 2024 • edited Loading

masad-frost May 11, 2024

Choose a reason for hiding this comment

masad-frost commented May 9, 2024 •

edited

Loading

masad-frost commented May 10, 2024 •

edited

Loading