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

Add basic handshake negotiation #341

Merged
merged 8 commits into from
Feb 19, 2020
Merged

Add basic handshake negotiation #341

merged 8 commits into from
Feb 19, 2020

Conversation

alecgibson
Copy link
Collaborator

@alecgibson alecgibson commented Jan 30, 2020

This change updates how the client and server communicate when first
connecting, in a non-breaking way.

Current behaviour

  • client creates and connects to a socket
  • client constructs a Connection object with the socket
  • the backend binds to the socket and constructs an Agent
  • the Agent sends an init message down the socket when constructed
  • the Connection receives the init message and initialises itself
    using information in the message

Motivation for change

There are currently a few things that would be nice to "negotiate" when
a client connects. For example, clients may wish to:

  • the same src value when reconnecting, so that it persists as a
    stable "session ID" (which has been implemented here)
  • set their default type
  • establish the exact versions of OT types being used
  • establish plugins and versions to use

More detail can be found here: #337

New behaviour

  • client creates and connects to a socket
  • client constructs a Connection object with the socket
  • the backend binds to the socket and constructs an Agent
  • the Agent sends an init message down the socket when constructed
  • the Connection checks the init message to see if the backend is
    capable of handshaking
  • if the backend cannot handshake (ie it's old), then the client
    initialises itself as before
  • new behaviour starts here
  • if the backend can handshake, then it will disregard the init
    message, and send its own hs "handshake" message to the server
  • the backend receives the handshake message and sends a response
  • the Connection receives the new hs message and initialises itself
    using information in the message (like before, but with more or
    different information information)

Other changes

As a convenience for some tests, which had to be tweaked (because they
assumed the connection was established immediately), this change also
adds an optional callback parameter to Backend.connect, which will
be invoked once the connection handshake completes.

We also add a guard for arbitrarily large seq, which will no longer be
reset on reconnection, so might hit integer overflow on particularly
long-lived Connection objects.

Alec Gibson added 3 commits January 30, 2020 17:32
This change updates how the client and server communicate when first
connecting, in a non-breaking way.

## Current behaviour

  - client creates and connects to a socket
  - client constructs a `Connection` object with the socket
  - the backend binds to the socket and constructs an `Agent`
  - the `Agent` sends an `init` message down the socket when constructed
  - the `Connection` receives the `init` message and initialises itself
    using information in the message

## Motivation for change

There are currently a few things that would be nice to "negotiate" when
a client connects. For example, clients may wish to:

  - the same `src` value when reconnecting, so that it persists as a
    stable "session ID"
  - set their default type
  - establish the exact versions of OT types being used
  - establish plugins and versions to use

More detail can be found here: #337

## New behaviour

  - client creates and connects to a socket
  - client constructs a `Connection` object with the socket
  - the backend binds to the socket and constructs an `Agent`
  - the `Agent` sends an `init` message down the socket when constructed
  - **new behaviour starts here**
  - the `Connection` checks the `init` message to see if the backend is
    capable of handshaking
  - if the backend _cannot_ handshake (ie it's old), then the client
    initialises itself as before
  - if the backend _can_ handshake, then it will disregard the `init`
    message, and send its own `hs` "handshake" message to the server
  - the backend receives the handshake message and sends a response
  - the `Connection` receives the new `hs` message and initialises itself
    using information in the message (like before, but with more or
    different information information)

## Other changes

No changes have been made to the information being shared. In other
words, this commit only updates the way in which the backend and the
client communicate when first connecting.

As a convenience for some tests, which had to be tweaked (because they
assumed the connection was established immediately), this change also
adds an optional `callback` parameter to `Backend.connect`, which will
be invoked once the connection handshake completes.
Right now, whenever a `Connection` is disconnected it:

  - sets its ID to `null`
  - resets its `seq` to `1`
  - clears its `Agent`

This makes it difficult for the server, and for remote clients, to keep
track of who is represented by a given ID (or `src`).

This change uses the new handshake protocol to allow the client to keep
its existing ID, so that it can persist even across a reconnection.
Previously, `seq` would get reset every time a client reconnects.
Whilst not entirely robust, it did help to mitigate the possibility that
very long-lived connections might hit an integer overflow when
incrementing their `seq`.

This change adds a guard against this case, and throws a sensible error,
both on the client, and on the server.

Note that we cannot rely on the client's `Number.MAX_SAFE_INTEGER`,
because it is not supported by Internet Explorer.
@coveralls
Copy link

coveralls commented Jan 30, 2020

Coverage Status

Coverage increased (+0.07%) to 96.851% when pulling 4b3ff13 on handshake into 7f9d98f on master.

lib/backend.js Outdated Show resolved Hide resolved
lib/backend.js Outdated Show resolved Hide resolved
lib/client/connection.js Outdated Show resolved Hide resolved
lib/backend.js Outdated Show resolved Hide resolved
lib/client/connection.js Outdated Show resolved Hide resolved
lib/backend.js Outdated Show resolved Hide resolved
Alec Gibson added 5 commits February 5, 2020 17:51
This change moves the handshake initialisation to the socket's `onopen`.
This avoids us having to wait for a message from the server before
initiating the handshake, saving us half a round-trip. It should also
make it easier to eventually refactor out the legacy initialisation.
This change adds an `Agent.src` field, which will accept a
client-provided ID. This is kept separate from the Agent-generated
`clientId` so that the agent's ID does not change during the handshake
(which could confuse any consumers relying on a stable client ID in the
`connect` middleware).

Any internal references to `clientId` then give precedence to the
consumer-provided `src`. If it hasn't been set (ie we're dealing with a
legacy client), then we fall back to using `clientId`.
This change adds a "legacy" version of ShareDB as a development
dependency so that we can unit test the new handshake logic against old
clients.
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.

3 participants