[protocol] Pre-session handshake #53

jackyzha0 · 2024-02-20T22:22:31Z

Why

Having a pre-session handshake to exchange information is useful! Currently, we do this implicitly on the first message of the connection but explicit good 👍

What changed

Separated out client/server transport into sub-abstract classes of transport to reduce boilerplate in sending/receiving handshake sequence
Implement the pre-session handshake (connection isn't turned into session until the handshake is completed)

jackyzha0 · 2024-02-20T22:30:05Z

router/context.ts

-export interface ServiceContext {
-  state: object | unknown;
-}
+export interface ServiceContext {}


idk why this included state before, we have a separate type for this

masad-frost · 2024-02-22T19:30:34Z

transport/message.ts

  type: Type.Literal('CLOSE'),
 });

+export const PROTOCOL_VERSION = 'v1';


masad-frost · 2024-02-23T01:01:29Z

transport/impls/stdio/client.ts

@@ -36,15 +36,14 @@ export class StdioClientTransport extends Transport<StreamConnection> {
  async createNewOutgoingConnection(to: TransportClientId) {
    log?.info(`${this.clientId} -- establishing a new stream to ${to}`);
    const conn = new StreamConnection(this.input, this.output);
-    conn.onData((data) => this.handleMsg(this.parseMsg(data)));
+    conn.onData(this.receiveWithBootSequence(conn));
    const cleanup = () => {
      this.onDisconnect(conn, to);


Mentioned in the other PR

#52 (comment)

Maybe I'm missing something, but why is the implementer responsible for calling onConnect and onDisconnect? Why don't they just call connect and which calls the implemented createNewOutgoingConnection and returns a Connection. Then inside connect we use the returned connection and pass it to onConnect and onDisconnect, this would probably require us to add more event listeners to the Connection interface like i mentioned in another comment.

lhchavez

left a big comment that combines semantic neatness and also addresses pid2 version drift for the first release of this.

transport/impls/stdio/server.ts

… .onConnect and .onDisconnect

lhchavez · 2024-02-28T18:45:52Z

transport/transport.ts

+              this.clientId
+            } -- received invalid handshake resp: ${JSON.stringify(parsed)}`,
+          );
+          return;


should we really return here? what happens in the scenario i outlined in the main PR? (multiplayers, one workspace is older than the other, both connecting to a newer or older pid2)

~~hm I can't find the comment in the main PR you are referring to, can you reiterate?~~ oh nvm the comment in this pr haha found it

imo we shouldn't worry about mismatched river versions until after this PR lands

not receiving a handshake message should be treated as fatal/refuse the connection and will help us find mistakes faster

pid2 flag audience is small enough (3 people) that just refreshing client for now is fine

for ai chat, we can bake that compatability into the python river server

oh so essentially implement what i'm suggesting but only in ai chat. that seems reasonable, although i'd like us to start to build the backwards-compatibility muscle before it starts becoming load-bearing.

yeah that makes sense, i think once this change is in place i'd be more more comfortable as this makes backwards-compatability much easier and less brittle than what we have now 👍

masad-frost · 2024-02-29T00:01:01Z

transport/session.ts

  abstract addDataListener(cb: (msg: Uint8Array) => void): void;
+  abstract removeDataListener(cb: (msg: Uint8Array) => void): void;


I prefer the disposable pattern where we return a handle to unlisten, just feels more ergonomic, no strong opinion.

masad-frost

Looks good. Left some comments.

One meta comment is that I don't feel like I have a good grasp on how we deal with errors, logging is fine for observability, but it doesn't allow the consumer to handle things or at least receive a signal that things are absolutely broken. Maybe downstream from that, still feels like we can be asserting more and be less forgiving with undefined states.

transport/transport.ts

masad-frost · 2024-02-29T00:24:00Z

transport/transport.ts

+            parsed,
+          )}`,
+        );
+        return;


hmm, seems like we have a ton of clean up here that we're not doing? Should we be terminating the connection after we send?

or is the idea that there's a recovery path here?

hm yeah idea is that client might send the proper message at some point in the future, though maybe that's bad anyways and we should just close the conn?

transport/session.ts

masad-frost · 2024-02-29T00:35:33Z

transport/impls/uds/connection.ts

+  }
+
+  addErrorListener(cb: (err: Error) => void): void {
+    this.sock.on('error', cb);


https://nodejs.org/api/net.html#event-error

Maybe we need to call sock.close here?

I believe that's only for the actual net.Server itself, sock is net.Socket so it will call both close and error

transport/transport.ts

masad-frost · 2024-02-29T00:40:24Z

transport/transport.ts

+        throw new Error(
+          `${this.clientId} -- connection to ${to} failed after ${attempt} attempts (${err}), giving up`,
+        );


Should probably log this. Also serializing err here might be painful to read or get good information from.

masad-frost · 2024-02-29T00:40:46Z

transport/transport.ts

+        log?.warn(
+          `${this.clientId} -- connection to ${to} failed (${err}), trying again in ${backoffMs}ms`,
+        );
+        setTimeout(() => this.connect(to, attempt + 1), backoffMs);


any checks we need to do before blindly calling this.connect?

nope, this.connect maintains its own invariants!

jackyzha0 commented Feb 20, 2024

View reviewed changes

jackyzha0 requested a review from lhchavez February 20, 2024 22:31

jackyzha0 marked this pull request as ready for review February 20, 2024 22:31

masad-frost reviewed Feb 22, 2024

View reviewed changes

transport/message.ts

type: Type.Literal('CLOSE'),

});

export const PROTOCOL_VERSION = 'v1';

Copy link

Member

masad-frost Feb 22, 2024

Choose a reason for hiding this comment

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

❤️

masad-frost reviewed Feb 23, 2024

View reviewed changes

jackyzha0 mentioned this pull request Feb 23, 2024

[protocol] distinguish between connections and sessions #52

Merged

jackyzha0 force-pushed the jackyzha0/handshake branch from 82f25d2 to a6ff717 Compare February 23, 2024 01:55

lhchavez reviewed Feb 26, 2024

View reviewed changes

transport/impls/stdio/server.ts Outdated Show resolved Hide resolved

jackyzha0 force-pushed the jackyzha0/handshake branch from b89e76f to 835f458 Compare February 27, 2024 21:43

Base automatically changed from jackyzha0/distinguish-sessions-and-connectios to main February 27, 2024 21:45

jackyzha0 added 8 commits February 27, 2024 14:00

distinguish client and server transports

c781067

boot sequence

8f9bcbf

remove redundant createNewConnection into server transports

6c6b087

allow ci everywhere

e85e7b0

i hate yaml

4d75f41

just run push on main

7a98590

test all

90be27a

fmt

5543ed1

jackyzha0 force-pushed the jackyzha0/handshake branch from 835f458 to 5543ed1 Compare February 27, 2024 22:00

jackyzha0 added 2 commits February 27, 2024 15:26

create handleConnection abstraction so implementors dont need to call…

ae72dbd

… .onConnect and .onDisconnect

update docs

3578c55

jackyzha0 requested review from lhchavez and masad-frost February 27, 2024 23:42

remove handshake bit

3c54c65

lhchavez reviewed Feb 28, 2024

View reviewed changes

make semantics of boot handler vs non-boot handler more clear

c84df64

masad-frost reviewed Feb 29, 2024

View reviewed changes

masad-frost approved these changes Feb 29, 2024

View reviewed changes

address review comments

30745da

jackyzha0 merged commit 076e35c into main Feb 29, 2024
3 checks passed

jackyzha0 deleted the jackyzha0/handshake branch February 29, 2024 05:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[protocol] Pre-session handshake #53

[protocol] Pre-session handshake #53

jackyzha0 commented Feb 20, 2024 •

edited

Loading

jackyzha0 Feb 20, 2024

masad-frost Feb 22, 2024

masad-frost Feb 23, 2024

masad-frost Feb 23, 2024

lhchavez left a comment

lhchavez Feb 28, 2024

jackyzha0 Feb 28, 2024 •

edited

Loading

jackyzha0 Feb 28, 2024

lhchavez Feb 28, 2024

jackyzha0 Feb 28, 2024

masad-frost Feb 29, 2024

masad-frost left a comment

masad-frost Feb 29, 2024

masad-frost Feb 29, 2024

jackyzha0 Feb 29, 2024

masad-frost Feb 29, 2024

jackyzha0 Feb 29, 2024

masad-frost Feb 29, 2024

masad-frost Feb 29, 2024

jackyzha0 Feb 29, 2024

		abstract addDataListener(cb: (msg: Uint8Array) => void): void;
		abstract removeDataListener(cb: (msg: Uint8Array) => void): void;

[protocol] Pre-session handshake #53

[protocol] Pre-session handshake #53

Conversation

jackyzha0 commented Feb 20, 2024 • edited Loading

Why

What changed

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

lhchavez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackyzha0 Feb 28, 2024 • edited Loading

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

jackyzha0 commented Feb 20, 2024 •

edited

Loading

jackyzha0 Feb 28, 2024 •

edited

Loading