From f2827f6409b2ceedcae029190eb39fdc23f230fd Mon Sep 17 00:00:00 2001 From: David Sheldrick Date: Thu, 2 May 2024 14:54:21 +0100 Subject: [PATCH] Allow clients to gracefully handle rejection (#3673) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR fixes the issue where sync clients would get into a reconnect loop after being rejected by the sync server. - Close the socket when in the error state (see useRemoteSyncClient) - Show a 'plx refresh the page' screen that doesn't have a sad face on it. image - If older clients who can't handle rejection well need to be rejected (e.g. due to a store migration being added) then we send them to a special purgatory where the canvas goes blank and it shows the offline indicator but the websocket connection stays open and it won't try to reconnect. ### Change Type - [x] `dotcom` — Changes the tldraw.com web app - [x] `bugfix` — Bug fix ### Test Plan 1. Gonna manually test this one by doing sneaky deploys to a test PR --- .../src/components/ErrorPage/ErrorPage.tsx | 37 +++++--- .../src/components/StoreErrorScreen.tsx | 24 +++++- apps/dotcom/src/hooks/useRemoteSyncClient.ts | 6 +- apps/dotcom/src/pages/history-snapshot.tsx | 1 - apps/dotcom/src/pages/history.tsx | 1 - apps/dotcom/src/pages/new.tsx | 1 - apps/dotcom/src/pages/not-found.tsx | 1 - .../ClientWebSocketAdapter.test.ts | 4 +- packages/tlsync/src/index.ts | 2 +- packages/tlsync/src/lib/RoomSession.ts | 1 - packages/tlsync/src/lib/TLSyncClient.ts | 4 +- packages/tlsync/src/lib/TLSyncRoom.ts | 33 +++---- packages/tlsync/src/lib/protocol.ts | 6 +- packages/tlsync/src/test/TLServer.test.ts | 4 +- .../tlsync/src/test/upgradeDowngrade.test.ts | 86 +++++++++++++++---- 15 files changed, 143 insertions(+), 68 deletions(-) diff --git a/apps/dotcom/src/components/ErrorPage/ErrorPage.tsx b/apps/dotcom/src/components/ErrorPage/ErrorPage.tsx index ba5aebb13..2565c8a86 100644 --- a/apps/dotcom/src/components/ErrorPage/ErrorPage.tsx +++ b/apps/dotcom/src/components/ErrorPage/ErrorPage.tsx @@ -1,28 +1,39 @@ +import { ReactNode } from 'react' import { Link } from 'react-router-dom' import { isInIframe } from '../../utils/iFrame' -export function ErrorPage({ - icon, - messages, -}: { - icon?: boolean - messages: { header: string; para1: string; para2?: string } -}) { +const GoBackLink = () => { const inIframe = isInIframe() + return ( + + {inIframe ? 'Open tldraw.' : 'Back to tldraw.'} + + ) +} + +const sadFaceIcon = ( + +) + +export function ErrorPage({ + messages, + icon = sadFaceIcon, + cta = , +}: { + icon?: ReactNode + messages: { header: string; para1: string; para2?: string } + cta?: ReactNode +}) { return (
- {icon && ( - {'Not - )} + {icon}

{messages.header}

{messages.para1}

{messages.para2 &&

{messages.para2}

}
- - {inIframe ? 'Open tldraw.' : 'Back to tldraw.'} - + {cta}
) diff --git a/apps/dotcom/src/components/StoreErrorScreen.tsx b/apps/dotcom/src/components/StoreErrorScreen.tsx index 6694bdbf1..13fdc65e9 100644 --- a/apps/dotcom/src/components/StoreErrorScreen.tsx +++ b/apps/dotcom/src/components/StoreErrorScreen.tsx @@ -1,17 +1,33 @@ import { TLIncompatibilityReason } from '@tldraw/tlsync' import { exhaustiveSwitchError } from 'tldraw' +import 'tldraw/tldraw.css' import { RemoteSyncError } from '../utils/remote-sync/remote-sync' import { ErrorPage } from './ErrorPage/ErrorPage' export function StoreErrorScreen({ error }: { error: Error }) { let header = 'Could not connect to server.' let message = '' - if (error instanceof RemoteSyncError) { switch (error.reason) { case TLIncompatibilityReason.ClientTooOld: { - message = 'This client is out of date. Please refresh the page.' - break + return ( + + } + messages={{ + header: 'Refresh the page', + para1: 'You need to update to the latest version of tldraw to continue.', + }} + cta={} + /> + ) } case TLIncompatibilityReason.ServerTooOld: { message = @@ -38,5 +54,5 @@ export function StoreErrorScreen({ error }: { error: Error }) { } } - return + return } diff --git a/apps/dotcom/src/hooks/useRemoteSyncClient.ts b/apps/dotcom/src/hooks/useRemoteSyncClient.ts index d01df1fc8..c241cd705 100644 --- a/apps/dotcom/src/hooks/useRemoteSyncClient.ts +++ b/apps/dotcom/src/hooks/useRemoteSyncClient.ts @@ -40,7 +40,11 @@ export function useRemoteSyncClient(opts: UseSyncClientConfig): RemoteTLStoreWit const store = useTLStore({ schema }) + const error: NonNullable['error'] = state?.error ?? undefined + useEffect(() => { + if (error) return + const userPreferences = computed<{ id: string; color: string; name: string }>( 'userPreferences', () => { @@ -107,7 +111,7 @@ export function useRemoteSyncClient(opts: UseSyncClientConfig): RemoteTLStoreWit client.close() socket.close() } - }, [prefs, roomId, store, uri]) + }, [prefs, roomId, store, uri, error]) return useValue( 'remote synced store', diff --git a/apps/dotcom/src/pages/history-snapshot.tsx b/apps/dotcom/src/pages/history-snapshot.tsx index 90d861608..49b30d1de 100644 --- a/apps/dotcom/src/pages/history-snapshot.tsx +++ b/apps/dotcom/src/pages/history-snapshot.tsx @@ -28,7 +28,6 @@ export function Component() { if (!result || !result.timestamp) return ( { type: 'connect', connectRequestId: 'test', schema: { schemaVersion: 1, storeVersion: 0, recordVersions: {} }, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), lastServerClock: 0, } diff --git a/packages/tlsync/src/index.ts b/packages/tlsync/src/index.ts index accdc1109..578ad6112 100644 --- a/packages/tlsync/src/index.ts +++ b/packages/tlsync/src/index.ts @@ -29,7 +29,7 @@ export { } from './lib/diff' export { TLIncompatibilityReason, - TLSYNC_PROTOCOL_VERSION, + getTlsyncProtocolVersion, type TLConnectRequest, type TLPingRequest, type TLPushRequest, diff --git a/packages/tlsync/src/lib/RoomSession.ts b/packages/tlsync/src/lib/RoomSession.ts index b4b3f45bc..69c25be0a 100644 --- a/packages/tlsync/src/lib/RoomSession.ts +++ b/packages/tlsync/src/lib/RoomSession.ts @@ -33,7 +33,6 @@ export type RoomSession = state: typeof RoomSessionState.Connected sessionKey: string presenceId: string - isV4Client: boolean socket: TLRoomSocket serializedSchema: SerializedSchema lastInteractionTime: number diff --git a/packages/tlsync/src/lib/TLSyncClient.ts b/packages/tlsync/src/lib/TLSyncClient.ts index 7f64c0c09..fe63d6876 100644 --- a/packages/tlsync/src/lib/TLSyncClient.ts +++ b/packages/tlsync/src/lib/TLSyncClient.ts @@ -15,10 +15,10 @@ import { interval } from './interval' import { TLIncompatibilityReason, TLPushRequest, - TLSYNC_PROTOCOL_VERSION, TLSocketClientSentEvent, TLSocketServerSentDataEvent, TLSocketServerSentEvent, + getTlsyncProtocolVersion, } from './protocol' import './requestAnimationFrame.polyfill' @@ -272,7 +272,7 @@ export class TLSyncClient = Store type: 'connect', connectRequestId: this.latestConnectRequestId, schema: this.store.schema.serialize(), - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), lastServerClock: this.lastServerClock, }) } diff --git a/packages/tlsync/src/lib/TLSyncRoom.ts b/packages/tlsync/src/lib/TLSyncRoom.ts index 02f023efe..1c9cc56c0 100644 --- a/packages/tlsync/src/lib/TLSyncRoom.ts +++ b/packages/tlsync/src/lib/TLSyncRoom.ts @@ -41,10 +41,10 @@ import { import { interval } from './interval' import { TLIncompatibilityReason, - TLSYNC_PROTOCOL_VERSION, TLSocketClientSentEvent, TLSocketServerSentDataEvent, TLSocketServerSentEvent, + getTlsyncProtocolVersion, } from './protocol' /** @public */ @@ -420,9 +420,7 @@ export class TLSyncRoom { } else { if (session.debounceTimer === null) { // this is the first message since the last flush, don't delay it - session.socket.sendMessage( - session.isV4Client ? message : { type: 'data', data: [message] } - ) + session.socket.sendMessage({ type: 'data', data: [message] }) session.debounceTimer = setTimeout( () => this._flushDataMessages(sessionKey), @@ -449,14 +447,7 @@ export class TLSyncRoom { session.debounceTimer = null if (session.outstandingDataMessages.length > 0) { - if (session.isV4Client) { - // v4 clients don't support the "data" message, so we need to send each message separately - for (const message of session.outstandingDataMessages) { - session.socket.sendMessage(message) - } - } else { - session.socket.sendMessage({ type: 'data', data: session.outstandingDataMessages }) - } + session.socket.sendMessage({ type: 'data', data: session.outstandingDataMessages }) session.outstandingDataMessages.length = 0 } } @@ -678,14 +669,15 @@ export class TLSyncRoom { // if the protocol versions don't match, disconnect the client // we will eventually want to try to make our protocol backwards compatible to some degree // and have a MIN_PROTOCOL_VERSION constant that the TLSyncRoom implements support for - const isV4Client = message.protocolVersion === 4 && TLSYNC_PROTOCOL_VERSION === 5 - if ( - message.protocolVersion == null || - (message.protocolVersion < TLSYNC_PROTOCOL_VERSION && !isV4Client) - ) { + let theirProtocolVersion = message.protocolVersion + // 5 is the same as 6 + if (theirProtocolVersion === 5) { + theirProtocolVersion = 6 + } + if (theirProtocolVersion == null || theirProtocolVersion < getTlsyncProtocolVersion()) { this.rejectSession(session, TLIncompatibilityReason.ClientTooOld) return - } else if (message.protocolVersion > TLSYNC_PROTOCOL_VERSION) { + } else if (theirProtocolVersion > getTlsyncProtocolVersion()) { this.rejectSession(session, TLIncompatibilityReason.ServerTooOld) return } @@ -711,7 +703,6 @@ export class TLSyncRoom { state: RoomSessionState.Connected, sessionKey: session.sessionKey, presenceId: session.presenceId, - isV4Client, socket: session.socket, serializedSchema: sessionSchema, lastInteractionTime: Date.now(), @@ -751,7 +742,7 @@ export class TLSyncRoom { type: 'connect', connectRequestId: message.connectRequestId, hydrationType: 'wipe_all', - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: this.schema.serialize(), serverClock: this.clock, diff: migrated.value, @@ -797,7 +788,7 @@ export class TLSyncRoom { connectRequestId: message.connectRequestId, hydrationType: 'wipe_presence', schema: this.schema.serialize(), - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), serverClock: this.clock, diff: migrated.value, }) diff --git a/packages/tlsync/src/lib/protocol.ts b/packages/tlsync/src/lib/protocol.ts index 588eefa22..593083a03 100644 --- a/packages/tlsync/src/lib/protocol.ts +++ b/packages/tlsync/src/lib/protocol.ts @@ -2,7 +2,11 @@ import { SerializedSchema, UnknownRecord } from '@tldraw/store' import { NetworkDiff, ObjectDiff, RecordOpType } from './diff' /** @public */ -export const TLSYNC_PROTOCOL_VERSION = 5 +const TLSYNC_PROTOCOL_VERSION = 6 + +export function getTlsyncProtocolVersion() { + return TLSYNC_PROTOCOL_VERSION +} /** @public */ export const TLIncompatibilityReason = { diff --git a/packages/tlsync/src/test/TLServer.test.ts b/packages/tlsync/src/test/TLServer.test.ts index 523cae129..57e14b247 100644 --- a/packages/tlsync/src/test/TLServer.test.ts +++ b/packages/tlsync/src/test/TLServer.test.ts @@ -14,7 +14,7 @@ import { DBLoadResult, TLServer } from '../lib/TLServer' import { RoomSnapshot } from '../lib/TLSyncRoom' import { chunk } from '../lib/chunk' import { RecordOpType } from '../lib/diff' -import { TLSYNC_PROTOCOL_VERSION, TLSocketClientSentEvent } from '../lib/protocol' +import { TLSocketClientSentEvent, getTlsyncProtocolVersion } from '../lib/protocol' import { RoomState } from '../lib/server-types' // Because we are using jsdom in this package, jest tries to load the 'browser' version of the ws library @@ -145,7 +145,7 @@ describe('TLServer', () => { type: 'connect', lastServerClock: 0, connectRequestId: 'test-connect-request-id', - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema, } diff --git a/packages/tlsync/src/test/upgradeDowngrade.test.ts b/packages/tlsync/src/test/upgradeDowngrade.test.ts index 290d71d1a..89f4577da 100644 --- a/packages/tlsync/src/test/upgradeDowngrade.test.ts +++ b/packages/tlsync/src/test/upgradeDowngrade.test.ts @@ -15,12 +15,24 @@ import { RoomSnapshot, TLRoomSocket } from '../lib/TLSyncRoom' import { RecordOpType, ValueOpType } from '../lib/diff' import { TLIncompatibilityReason, - TLSYNC_PROTOCOL_VERSION, TLSocketServerSentEvent, + getTlsyncProtocolVersion, } from '../lib/protocol' import { TestServer } from './TestServer' import { TestSocketPair } from './TestSocketPair' +const actualProtocol = jest.requireActual('../lib/protocol') + +jest.mock('../lib/protocol', () => { + const actual = jest.requireActual('../lib/protocol') + return { + ...actual, + getTlsyncProtocolVersion: jest.fn(actual.getTlsyncProtocolVersion), + } +}) + +const mockGetTlsyncProtocolVersion = getTlsyncProtocolVersion as jest.Mock + function mockSocket(): TLRoomSocket { return { isOpen: true, @@ -328,7 +340,7 @@ test('clients will receive updates from a snapshot migration upon connection', ( type: 'connect', connectRequestId: 'test', lastServerClock: snapshot.clock, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV3.serialize(), }) @@ -352,7 +364,7 @@ test('out-of-date clients will receive incompatibility errors', () => { type: 'connect', connectRequestId: 'test', lastServerClock: 0, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV2.serialize(), }) @@ -363,6 +375,38 @@ test('out-of-date clients will receive incompatibility errors', () => { }) test('clients using an out-of-date protocol will receive compatibility errors', () => { + const actualVersion = getTlsyncProtocolVersion() + mockGetTlsyncProtocolVersion.mockReturnValue(actualVersion + 1) + try { + const v2server = new TestServer(schemaV2) + + const id = 'test_upgrade_v3' + const socket = mockSocket() + + v2server.room.handleNewSession(id, socket) + v2server.room.handleMessage(id, { + type: 'connect', + connectRequestId: 'test', + lastServerClock: 0, + protocolVersion: actualVersion, + schema: schemaV2.serialize(), + }) + + expect(socket.sendMessage).toHaveBeenCalledWith({ + type: 'incompatibility_error', + reason: TLIncompatibilityReason.ClientTooOld, + }) + } finally { + mockGetTlsyncProtocolVersion.mockReset() + mockGetTlsyncProtocolVersion.mockImplementation(actualProtocol.getTlsyncProtocolVersion) + } +}) + +// this can be deleted when the protocol gets to v7 +test('v5 special case should allow connections', () => { + const actualVersion = getTlsyncProtocolVersion() + if (actualVersion > 6) return + const v2server = new TestServer(schemaV2) const id = 'test_upgrade_v3' @@ -373,13 +417,23 @@ test('clients using an out-of-date protocol will receive compatibility errors', type: 'connect', connectRequestId: 'test', lastServerClock: 0, - protocolVersion: TLSYNC_PROTOCOL_VERSION - 2, + protocolVersion: 5, schema: schemaV2.serialize(), }) expect(socket.sendMessage).toHaveBeenCalledWith({ - type: 'incompatibility_error', - reason: TLIncompatibilityReason.ClientTooOld, + connectRequestId: 'test', + diff: {}, + hydrationType: 'wipe_all', + protocolVersion: 6, + schema: { + schemaVersion: 2, + sequences: { + 'com.tldraw.user': 1, + }, + }, + serverClock: 1, + type: 'connect', }) }) @@ -394,7 +448,7 @@ test('clients using a too-new protocol will receive compatibility errors', () => type: 'connect', connectRequestId: 'test', lastServerClock: 0, - protocolVersion: TLSYNC_PROTOCOL_VERSION + 1, + protocolVersion: getTlsyncProtocolVersion() + 1, schema: schemaV2.serialize(), }) @@ -436,7 +490,7 @@ test('when the client is too new it cannot connect', () => { type: 'connect', connectRequestId: 'test', lastServerClock: 10, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV2.serialize(), }) @@ -496,7 +550,7 @@ describe('when the client is too old', () => { type: 'connect', connectRequestId: 'test', lastServerClock: 10, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV1.serialize(), }) @@ -505,7 +559,7 @@ describe('when the client is too old', () => { type: 'connect', connectRequestId: 'test', lastServerClock: 10, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV2.serialize(), }) @@ -514,7 +568,7 @@ describe('when the client is too old', () => { connectRequestId: 'test', hydrationType: 'wipe_presence', diff: {}, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV2.serialize(), serverClock: 10, } satisfies TLSocketServerSentEvent) @@ -524,7 +578,7 @@ describe('when the client is too old', () => { connectRequestId: 'test', hydrationType: 'wipe_presence', diff: {}, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV2.serialize(), serverClock: 10, } satisfies TLSocketServerSentEvent) @@ -643,7 +697,7 @@ describe('when the client is the same version', () => { type: 'connect', connectRequestId: 'test', lastServerClock: 10, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: JSON.parse(JSON.stringify(schemaV2.serialize())), }) @@ -652,7 +706,7 @@ describe('when the client is the same version', () => { type: 'connect', connectRequestId: 'test', lastServerClock: 10, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: JSON.parse(JSON.stringify(schemaV2.serialize())), }) @@ -661,7 +715,7 @@ describe('when the client is the same version', () => { connectRequestId: 'test', hydrationType: 'wipe_presence', diff: {}, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV2.serialize(), serverClock: 10, } satisfies TLSocketServerSentEvent) @@ -671,7 +725,7 @@ describe('when the client is the same version', () => { connectRequestId: 'test', hydrationType: 'wipe_presence', diff: {}, - protocolVersion: TLSYNC_PROTOCOL_VERSION, + protocolVersion: getTlsyncProtocolVersion(), schema: schemaV2.serialize(), serverClock: 10, } satisfies TLSocketServerSentEvent)