Client uses ReadStream and WriteStream #136

masad-frost · 2024-05-21T21:33:58Z

Why

More protocolv2 work #127

What changed

In this PR it's a simple replacement of pushable interface with stream interfaces, on the client-side. No protocol or API changes otherwise. Streams API is a superset of pushable, many of the stream methods aren't implemented correctly/don't make a lot of sense without further changes, but this is a good step.

Versioning

Breaking protocol change
Breaking ts/js API change

Monkatraz

lgtm! just nitpicks or observations

router/client.ts

Monkatraz · 2024-05-21T22:19:00Z

router/client.ts

+          span.setStatus({ code: SpanStatusCode.OK });
+        },
+      );
+      const readStreamRequestCloseNotImplemented = () => void 0;


Monkatraz · 2024-05-21T22:20:23Z

util/testHelpers.ts

+  next(): Promise<
+    | {
+        done: false;
+        value: T;
+      }
+    | {
+        done: true;
+        value: undefined;
+      }
+  >;


maybe this should be its own type lol, seems kind of useful

I had it be part of the interfaces in streams.ts, but then just inlined it, we'll see if it comes up again.

Monkatraz · 2024-05-21T22:22:48Z

__tests__/cleanup.test.ts

@@ -364,10 +368,11 @@ describe.each(testMatrix())(
      });

      // start a stream
-      const [input, output] = await client.test.echo.stream();
-      input.push({ msg: '1', ignore: false });
+      const [inputWriter, outputReader] = await client.test.echo.stream();


I like you naming these inputWriter and outputReader (and on the server, inputReader and outputWriter), not too verbose but feels intuitive

I'd like to update type annotations to have those names, but maybe we can do that in a follow up since we reference 'input' and 'output' in a bunch of places

Monkatraz · 2024-05-21T22:24:32Z

util/testHelpers.ts

+export function getIteratorFromStream<T>(readStream: ReadStream<T>) {
+  return readStream[Symbol.asyncIterator]();
+}


I'm still kind of surprised there isn't something like Iterator.from(foo) in JS

seems like we should be able to just use the stream as an async iterator https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/asyncIterator without this helper?

the for-await-of function automatically calls this, if you want to call next directly, you have to grab the iterator like this

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

jackyzha0 · 2024-05-23T05:28:49Z

__tests__/bandwidth.bench.ts

-        const result = await output.next();
-        assert(result.value.ok);
+        inputWriter.write({ msg: nanoid(), ignore: false });
+        const result = await outputReader[Symbol.asyncIterator]().next();


does a bare await outputReader().next(); not work?

no, you need to grab the iterator

jackyzha0 · 2024-05-23T05:29:35Z

__tests__/cleanup.test.ts


-      const result1 = await iterNext(output);
+      const outputIterator = getIteratorFromStream(outputReader);


also when to use outputReader[Symbol.asyncIterator] vs getIteratorFromStream(outputReader)

it's just a helper, we can either remove getIteratorFromStream or i'll make sure we're using it everywhere.

jackyzha0 · 2024-05-23T05:33:47Z

util/testHelpers.ts

+export function getIteratorFromStream<T>(readStream: ReadStream<T>) {
+  return readStream[Symbol.asyncIterator]();
+}


seems like we should be able to just use the stream as an async iterator https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/asyncIterator without this helper?

* Server uses streams * Bruh

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

* Server uses streams * Bruh

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

* Server uses streams * Bruh

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

* Server uses streams * Bruh

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

* Server uses streams * Bruh

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

* Server uses streams * Bruh

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

* Server uses streams * Bruh

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

* Server uses streams * Bruh

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]>

masad-frost added 2 commits May 21, 2024 20:58

Client uses ReadStream

7071396

Client uses WriteStream

8565c0e

masad-frost requested a review from a team as a code owner May 21, 2024 21:33

masad-frost requested review from Monkatraz and removed request for a team May 21, 2024 21:33

masad-frost added 2 commits May 21, 2024 21:34

delete port config lol

9eea4d2

Disable everything for handler.test.ts

b133da7

Monkatraz approved these changes May 21, 2024

View reviewed changes

masad-frost mentioned this pull request May 21, 2024

Server uses ReadStream and WriteStream #136 #137

Merged

no void 0

23ba459

masad-frost merged commit 9be4fa0 into protocolv2 May 21, 2024
3 checks passed

masad-frost deleted the fm-client-use-streams branch May 21, 2024 23:33

masad-frost mentioned this pull request May 21, 2024

Base branch for protocol v2 #127

Merged

32 tasks

masad-frost added a commit that referenced this pull request May 21, 2024

Client uses ReadStream and WriteStream (#136)

ad39e56

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

jackyzha0 reviewed May 23, 2024

View reviewed changes

masad-frost added a commit that referenced this pull request May 28, 2024

Server uses ReadStream and WriteStream #136 (#137)

45072e5

* Server uses streams * Bruh

masad-frost added a commit that referenced this pull request May 29, 2024

Client uses ReadStream and WriteStream (#136)

c8d5762

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

masad-frost added a commit that referenced this pull request May 29, 2024

Server uses ReadStream and WriteStream #136 (#137)

be75882

* Server uses streams * Bruh

masad-frost added a commit that referenced this pull request May 31, 2024

Client uses ReadStream and WriteStream (#136)

5e0d558

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

masad-frost added a commit that referenced this pull request May 31, 2024

Server uses ReadStream and WriteStream #136 (#137)

682adad

* Server uses streams * Bruh

masad-frost added a commit that referenced this pull request Jun 6, 2024

Client uses ReadStream and WriteStream (#136)

1d22762

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

masad-frost added a commit that referenced this pull request Jun 6, 2024

Server uses ReadStream and WriteStream #136 (#137)

87b15ec

* Server uses streams * Bruh

masad-frost added a commit that referenced this pull request Jun 11, 2024

Client uses ReadStream and WriteStream (#136)

a4d0d0a

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

masad-frost added a commit that referenced this pull request Jun 11, 2024

Server uses ReadStream and WriteStream #136 (#137)

2143a34

* Server uses streams * Bruh

masad-frost added a commit that referenced this pull request Jun 13, 2024

Client uses ReadStream and WriteStream (#136)

d079bd0

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

masad-frost added a commit that referenced this pull request Jun 13, 2024

Server uses ReadStream and WriteStream #136 (#137)

c250646

* Server uses streams * Bruh

masad-frost added a commit that referenced this pull request Jun 20, 2024

Client uses ReadStream and WriteStream (#136)

ac1ec4e

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

masad-frost added a commit that referenced this pull request Jun 20, 2024

Server uses ReadStream and WriteStream #136 (#137)

7a92a81

* Server uses streams * Bruh

masad-frost added a commit that referenced this pull request Jun 24, 2024

Client uses ReadStream and WriteStream (#136)

b5bf962

* Client uses ReadStream * Client uses WriteStream * delete port config lol * Disable everything for handler.test.ts * no void 0

masad-frost added a commit that referenced this pull request Jun 24, 2024

Server uses ReadStream and WriteStream #136 (#137)

4542ee1

* Server uses streams * Bruh

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client uses ReadStream and WriteStream #136

Client uses ReadStream and WriteStream #136

masad-frost commented May 21, 2024 •

edited

Loading

Monkatraz left a comment

Monkatraz May 21, 2024

Monkatraz May 21, 2024

masad-frost May 21, 2024

Monkatraz May 21, 2024

masad-frost May 21, 2024

masad-frost May 21, 2024

Monkatraz May 21, 2024

jackyzha0 May 23, 2024

masad-frost May 23, 2024

jackyzha0 May 23, 2024

masad-frost May 23, 2024

jackyzha0 May 23, 2024

masad-frost May 23, 2024

jackyzha0 May 23, 2024


		const result1 = await iterNext(output);
		const outputIterator = getIteratorFromStream(outputReader);

Client uses ReadStream and WriteStream #136

Client uses ReadStream and WriteStream #136

Conversation

masad-frost commented May 21, 2024 • edited Loading

Why

What changed

Versioning

Monkatraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masad-frost commented May 21, 2024 •

edited

Loading