Refactor websocket adapters #248

HerbCaudill · 2023-12-04T14:34:47Z

Refactors BrowserWebSocketClientAdapter and NodeWSServerAdapter for readability and maintainability - see comments below.
Makes the retry and keepAlive intervals configurable, so we can speed them up in testing and get those processes covered by tests.
Handles the leave message on the server - see comments.
Implements the disconnect method on the server - see comments.
Refactors the package's test suites to simplify setup.
Adds tests to bring this package to 100% coverage.

HerbCaudill · 2023-12-04T14:36:53Z

packages/automerge-repo-network-websocket/src/BrowserWebSocketClientAdapter.ts

+      // first time connecting
+      this.#log("connecting")
+      this.peerId = peerId
+      this.peerMetadata = peerMetadata ?? {}


only set these on initial connection

HerbCaudill · 2023-12-04T14:37:24Z

packages/automerge-repo-network-websocket/src/BrowserWebSocketClientAdapter.ts

+      this.peerMetadata = peerMetadata ?? {}
+    } else {
+      this.#log("reconnecting")
+      assert(peerId === this.peerId)


...but ensure that the peerId isn't changed when reconnecting

HerbCaudill · 2023-12-04T14:44:29Z

packages/automerge-repo-network-websocket/src/BrowserWebSocketClientAdapter.ts

+  onError = (event: WebSocket.ErrorEvent) => {
+    const { code } = event.error
+    if (code === "ECONNREFUSED") {
+      this.#log("Connection refused, retrying...")
+    } else {
+      /* c8 ignore next */
+      throw event.error
    }
+  }


Previously we weren't handling socket errors at all, so they weren't ever surfaced anywhere. This ensures that they're properly thrown. I also have it retry if the server refuses the connection (e.g. it's still starting up). See this test:

automerge-repo/packages/automerge-repo-network-websocket/test/Websocket.test.ts

Lines 95 to 115 in 52572c4

it("should connect even when server is not initially available", async () => {

const port = await getPort() //?

const retryInterval = 100

const browserAdapter = await setupClient({ port, retryInterval })

const _browserRepo = new Repo({

network: [browserAdapter],

peerId: browserPeerId,

})

await pause(500)

const { serverAdapter } = await setupServer({ port, retryInterval })

const serverRepo = new Repo({

network: [serverAdapter],

peerId: serverPeerId,

})

await eventPromise(browserAdapter, "peer-candidate")

})

Much nicer, thanks. It's possible we'll want an exponential backoff at some point but I don't think it's critical at the moment.

HerbCaudill · 2023-12-04T14:46:23Z

packages/automerge-repo-network-websocket/src/BrowserWebSocketClientAdapter.ts

-    this.send({ type: "leave", senderId: this.peerId! })
+    assert(this.peerId)
+    assert(this.socket)
+    this.send({ type: "leave", senderId: this.peerId })


These messages are now handled on the server.

automerge-repo/packages/automerge-repo-network-websocket/src/NodeWSServerAdapter.ts

Lines 147 to 152 in 52572c4

} else if (isLeaveMessage(message)) {

const { senderId } = message

const socket = this.sockets[senderId]

/* c8 ignore next */

if (!socket) return

this.#terminate(socket as WebSocketWithIsAlive)

Will these messages crash an old server?

(We should test unrecognized message handling.)

HerbCaudill · 2023-12-04T14:47:41Z

packages/automerge-repo-network-websocket/src/BrowserWebSocketClientAdapter.ts

-    }
-    this.remotePeerId = peerId
-    this.emit("peer-candidate", { peerId, peerMetadata })
+  peerCandidate(remotePeerId: PeerId, peerMetadata: PeerMetadata) {


renamed announceConnection to peerCandidate

HerbCaudill · 2023-12-04T14:55:27Z

packages/automerge-repo-network-websocket/src/NodeWSServerAdapter.ts

-        // It doesn't seem like this gets called;
-        // we handle leaving in the socket close logic
-        // TODO: confirm this
-        // ?


The client adapter does send leave messages when .disconnect() is called.

HerbCaudill · 2023-12-04T14:56:23Z

packages/automerge-repo-network-websocket/src/assert.ts

Another candidate for a shared utilities package

HerbCaudill · 2023-12-04T14:58:56Z

packages/automerge-repo-network-websocket/test/Websocket.test.ts

        peerMetadata: { storageId: undefined, isEphemeral: true },
        supportedProtocolVersions: ["1"],
      })
    })

-    it.skip("should announce disconnections", async () => {
+    it("should announce disconnections", async () => {


new test (I'd taken a stab at it earlier). fixes #209

HerbCaudill · 2023-12-04T14:59:06Z

packages/automerge-repo-network-websocket/test/Websocket.test.ts

+      await eventPromise(browserAdapter, "peer-candidate")
+    })
+
+    it("should reconnect after being disconnected", async () => {


HerbCaudill · 2023-12-04T14:59:30Z

packages/automerge-repo-network-websocket/test/Websocket.test.ts

+      ])
+    })
+
+    it("should connect even when server is not initially available", async () => {


HerbCaudill · 2023-12-05T13:28:20Z

Current dependencies on/for this PR:

main
- PR Upgrade Vitest to v1 #243
  - PR Coverage config #244
    - PR useRepo tests #242
      - PR Refactor websocket adapters #248 👈
        
        PR Switch to pnpm #249
        
        PR Network adapters can send and receive arbitrary message types #250

This stack of pull requests is managed by Graphite.

HerbCaudill · 2023-12-28T19:03:13Z

merged with other stuff

pvh · 2024-01-03T18:29:39Z

packages/automerge-repo-network-websocket/src/BrowserWebSocketClientAdapter.ts

@@ -1,8 +1,9 @@
 import {
  NetworkAdapter,
-  type PeerId,


These type assertions allow imported type removal for transpilers without loading the full object graph. Is there a reason you took them out?

HerbCaudill commented Dec 4, 2023

View reviewed changes

HerbCaudill requested a review from alexjg December 4, 2023 16:27

HerbCaudill marked this pull request as ready for review December 4, 2023 16:28

HerbCaudill force-pushed the refactor-websocket-adapters branch from 0d07b62 to b919b1d Compare December 4, 2023 17:11

HerbCaudill changed the base branch from main to use-repo-tests December 5, 2023 13:28

HerbCaudill force-pushed the refactor-websocket-adapters branch from a311513 to af5dd03 Compare December 5, 2023 13:28

This was referenced Dec 5, 2023

Upgrade Vitest to v1 #243

Merged

Coverage config #244

Merged

This was referenced Dec 5, 2023

useRepo tests #242

Closed

Switch to pnpm #249

Merged

Network adapters can send and receive arbitrary message types #250

Closed

HerbCaudill force-pushed the use-repo-tests branch from 787f36e to 694b532 Compare December 6, 2023 19:50

HerbCaudill force-pushed the refactor-websocket-adapters branch from af5dd03 to 50db010 Compare December 6, 2023 19:50

HerbCaudill and others added 14 commits December 8, 2023 15:20

.gitignore

e9bf1d6

BrowserWebSocketClientAdapter.ts: extract TimeoutId type

9d160d4

move type to bottom

dd36ab3

rename variables for clarity

ee37028

refactor logging

e7d2575

add type guards

5b22191

create assert.ts

26ec48c

create toArrayBuffer.ts

b1c83ee

NodeWSServerAdapter: refactor

d811fce

BrowserWebSocketClientAdapter: refactor

514949f

Websocket tests: refactor & add tests

e3e0023

also remove error listener

8d2cf8e

remove .only

3664107

import BrowserWebSocketClientAdapter

3eeef12

HerbCaudill force-pushed the use-repo-tests branch from 694b532 to da69dfd Compare December 8, 2023 14:26

HerbCaudill force-pushed the refactor-websocket-adapters branch from 50db010 to 3eeef12 Compare December 8, 2023 14:26

HerbCaudill force-pushed the refactor-websocket-adapters branch from 3c2e71f to 637564f Compare December 28, 2023 16:51

HerbCaudill changed the base branch from use-repo-tests to main December 28, 2023 16:54

HerbCaudill force-pushed the refactor-websocket-adapters branch from 637564f to 3eeef12 Compare December 28, 2023 17:00

HerbCaudill closed this Dec 28, 2023

pvh reviewed Jan 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor websocket adapters #248

Refactor websocket adapters #248

HerbCaudill commented Dec 4, 2023

HerbCaudill Dec 4, 2023

HerbCaudill Dec 4, 2023

HerbCaudill Dec 4, 2023

pvh Jan 3, 2024

HerbCaudill Dec 4, 2023

pvh Jan 3, 2024

pvh Jan 3, 2024

HerbCaudill Dec 4, 2023 •

edited

Loading

HerbCaudill Dec 4, 2023

HerbCaudill Dec 4, 2023

HerbCaudill Dec 4, 2023

HerbCaudill Dec 4, 2023

HerbCaudill Dec 4, 2023

HerbCaudill commented Dec 5, 2023 •

edited

Loading

HerbCaudill commented Dec 28, 2023

pvh Jan 3, 2024

	it("should connect even when server is not initially available", async () => {
	const port = await getPort() //?
	const retryInterval = 100

	const browserAdapter = await setupClient({ port, retryInterval })

	const _browserRepo = new Repo({
	network: [browserAdapter],
	peerId: browserPeerId,
	})

	await pause(500)

	const { serverAdapter } = await setupServer({ port, retryInterval })
	const serverRepo = new Repo({
	network: [serverAdapter],
	peerId: serverPeerId,
	})

	await eventPromise(browserAdapter, "peer-candidate")
	})

	} else if (isLeaveMessage(message)) {
	const { senderId } = message
	const socket = this.sockets[senderId]
	/* c8 ignore next */
	if (!socket) return
	this.#terminate(socket as WebSocketWithIsAlive)

Refactor websocket adapters #248

Refactor websocket adapters #248

Conversation

HerbCaudill commented Dec 4, 2023

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

HerbCaudill Dec 4, 2023 • 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

Choose a reason for hiding this comment

HerbCaudill commented Dec 5, 2023 • edited Loading

HerbCaudill commented Dec 28, 2023

Choose a reason for hiding this comment

HerbCaudill Dec 4, 2023 •

edited

Loading

HerbCaudill commented Dec 5, 2023 •

edited

Loading