Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Base branch for protocol v2 #127

Merged
merged 87 commits into from
Aug 16, 2024
Merged

Base branch for protocol v2 #127

merged 87 commits into from
Aug 16, 2024

Conversation

masad-frost
Copy link
Member

@masad-frost masad-frost commented May 15, 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

.replit Outdated Show resolved Hide resolved
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Show resolved Hide resolved
masad-frost and others added 5 commits June 24, 2024 23:49
* Update PROTOCOL.md

* Update PROTOCOL.md

Co-authored-by: Jacky Zhao <[email protected]>

* 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 <[email protected]>
* ReadStream implementation

* remove tee for now

* Make the stream itself an iterable

* Implement return for iterator

* comment
Copy link
Member

@jackyzha0 jackyzha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

talked more IRL, going to think a bit more about the stream interface and ergonomics broadly

i think directionally very correct but could use a lot of simplification

README.md Outdated
input: Type.Object({ n: Type.Number() }),
output: Type.Object({ result: Type.Number() }),
requestData: Type.Object({ n: Type.Number() }),
responseData: Type.Object({ result: Type.Number() }),
errors: Type.Never(),
// note that a handler is unique per user RPC
async handler(ctx, { n }) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably needs an update too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same with await client.example.add.rpc below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs

PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Show resolved Hide resolved
PROTOCOL.md Show resolved Hide resolved
router/server.ts Show resolved Hide resolved
router/server.ts Show resolved Hide resolved
router/streams.ts Show resolved Hide resolved
router/streams.ts Outdated Show resolved Hide resolved
util/testHelpers.ts Outdated Show resolved Hide resolved
@masad-frost
Copy link
Member Author

good review, i'll be sending some PRs and we can sync up afterwards

Got some comments on streams being complicated, so this is a take on
simplified API.

- Less methods and states that people need to understand
- Merge interfaces into a single object (i.e. a `stream` call would have
both read and write capabilities on the same object)
## 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 -->
Partially to match gRPC nomeclature and to avoid confusion with AbortSignal/AbortController
@jackyzha0
Copy link
Member

its time.

@masad-frost
Copy link
Member Author

a couple of things left that we'll fix against main

@masad-frost masad-frost enabled auto-merge (squash) August 16, 2024 21:02
@masad-frost masad-frost merged commit c17b335 into main Aug 16, 2024
4 checks passed
@masad-frost masad-frost deleted the protocolv2 branch August 16, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants