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

[CAP-49] no connection serials #1110

Merged
merged 33 commits into from
Jan 30, 2023
Merged

Conversation

andydunstall
Copy link
Contributor

@andydunstall andydunstall commented Jan 4, 2023

Implements no connection serials as described by https://github.com/ably/specification/pull/88/files, and upgrade described by https://github.com/ably/realtime/issues/4216#issuecomment-1221407649.

Changes

Protocol version

Updates the ably protocol version to 2.0

Spec

  • RSC7a: replaces version string 1.2 with 2.0

Testing
This is already tested by rest/http.test.js and realtime/init.test.js which check the query string and headers are correct.

Resume with ATTACH

Each channel tracks the latest channelSerial its received. Unlike with connection serials we no longer check for duplicates by comparing serials. This was only needed for the old upgrade scheme which could potentially have duplicates. We wanted to keep the channelSerial format opaque rather than having to parse it to use for comparison (such as is could be a legacy serial or a time serial).

When resuming, instead of including a connectionSerial in the query (which leaves the realtime backend to look up the channel serial for each channel and resume), the client just re-attaches the channels including the channelSerial in the ATTACH message.

This also removes the checkAttachedMsgIndicators check, which will re-attach if the channel has not received an ATTACHED after reconnecting, which is now redundant given we always re-attach.

Spec

  • RTN15c6/RTN15c7: The client already sends an ATTACH on the new connection for channels in the ATTACHING and SUSPENDED states - so this just moves channels in ATTACHED states back to ATTACHING when we get a new connection so they re-attach
  • RTL4c1: Includes the tracked channelSerial in the ATTACH message
  • RTL15b: Updates RealtimeChannel.properties.channelSerial when receive MESSAGE/PRESENCE/ATTACHED actions and channelSerial is defined
  • RTP5a1: Clears RealtimeChannel.properties.channelSerial when the channel enters DETACHED/SUSPENDED/FAILED states
  • RTN15b2: Removes the connectionSerial query param on resume

Testing
realtime/resume.test.js already tests resume works under a number of scenarios

Presence Re-Enter

Spec

  • RTP17h: Stores own members by clientId only (rather than member key)
  • RTP17f: When the channel becomes attached RealtimePresence.onAttached is called (though not if already attached as its RealtimeChannel already checks state === this.state). When attached _ensureMyMembersPresent runs to perform re-entry
  • RTP17g:
    • Adds the member id attribute to the re-entry
    • No longer removes the member from the internal presence map
    • No longer checks that member key is not already a member of the main presence map

Testing

  • With the new presence model presence_auto_reenter now re-enters both members (even the member that was already in the presence set) and channel.presence.get returns the same member (one) twice with different connection IDs
    • @SimonWoolf I think when we talked about it before this is expected? (I guess before one wouldn't have been re-entered as it was in the main presence map?)

Recovery

The recover now needs to store the channelSerials for each attached channel (since this state is no longer kept by the realtime backend), which is encoded as JSON for simplicity rather than some custom format. This adds a new method createRecoveryKey to create the key rather than recalculating each time (which could get expensive). To keep compatibility also adds a recoveryKey getter that returns createRecoveryKey, which required updating tsconfig to target es5 (agreed in https://github.com/ably/specification/pull/88/files).

Spec

  • RTN16f: Already does this so is updated to decode the new recovery key format
  • RTN16j: recoverChannels sets the channelSerial for all channels in the recovery key before connecting
  • RTN16k: Already does this so is updated to decode the new recovery key format
  • RTN16g: Adds a createRecoveryKey which returns the msgSerial, connectionKey and name to channelSerial pairs for each channel, encoded as JSON
    • RTN16g2: Checks if it has a connection key, if not returns null (if CLOSED, CLOSING, FAILED, or SUSPENDED, connectionKey is cleared
  • RTN16d: Adds recover multiple channels which tests the conn id stays the same but the conn key changes after recover

Testing

  • Adds a recover multiple channels, which is similar to resume_active except uses recover rather than resume. This also tests the connection id stays the same but the connection key changes as described by RTN16d.

Upgrade

Implements the new upgrade scheme as described in option 1 of https://github.com/ably/realtime/issues/4216#issuecomment-1221407649 (not in the spec as ably-js specific). Also rather than keeping the SYNC action, which is also used by presence which causes some confusion, upgrade instead now uses the ACTIVATE action to activate the new transport.

  1. Before activating the new transport after an upgrade we send an empty ACTIVATE message to the server and assumes the transport is active immediately after sending (since we'll reattach so doesn't matter if we drop messages),
  2. When receiving the ACTIVATE the server sets the active transport and suspends the connections channels (the same as if it got a resume request),
  3. Since the SDK has activated a new transport it re-attaches all channels as it would on a normal resume

The SDK no longer waits for channels.onceNopending since we re-attach once the new connection is activated.

Testing

  • Already has lots of upgrade testing
  • Removes the unresponsive_upgrade_sync test as this no longer makes sense (since we don't wait for SYNC)

Spec

  • G4
  • RSC7a
  • RTN8c
  • RTN9c
  • RTN10
    • RTN10a
    • RTN10b
  • RTN15g3
  • RTN15b
    • RTN15b2
  • RTN15c
    • RTN15c1
    • RTN15c2
    • RTN15c3
    • RTN15c6
    • RTN15c7
  • RTN15f
  • RTN16
    • RTN16a
    • RTN16b
    • RTN16c
    • RTN16d
    • RTN16e
    • RTN16f
    • RTN16i
    • RTN16j
    • RTN16k
    • RTN16g
      • RTN16g1
      • RTN16g2
    • RTN16d
    • RTN16l
  • RTN19
    • RTN19a1
    • RTN19a2
  • RTN25
  • RTL4c1
  • RTL15b
  • RTP17
    • RTP17h
    • RTP17f
    • RTP17g
    • RTP17c
    • RTP17d
  • RTP5
    • RTP5a1
  • TR4f
  • CP2a
  • CP2b

@github-actions github-actions bot temporarily deployed to staging/pull/1110/typedoc January 4, 2023 17:54 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 4, 2023 17:54 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 5, 2023 08:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/typedoc January 5, 2023 08:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 5, 2023 11:33 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-no-connection-serials branch 2 times, most recently from 13f03fd to 61552d6 Compare January 5, 2023 13:52
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 5, 2023 13:53 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-no-connection-serials branch from 61552d6 to 9149e9f Compare January 5, 2023 14:22
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 5, 2023 14:24 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-no-connection-serials branch from 9149e9f to ad803b5 Compare January 5, 2023 14:33
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 5, 2023 14:34 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-no-connection-serials branch from ad803b5 to 5525f43 Compare January 6, 2023 08:22
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 6, 2023 08:24 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-no-connection-serials branch from 5525f43 to d18c613 Compare January 6, 2023 09:15
@github-actions github-actions bot temporarily deployed to staging/pull/1110/typedoc January 6, 2023 09:17 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 6, 2023 09:17 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 6, 2023 12:42 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/typedoc January 6, 2023 12:42 Inactive
@andydunstall andydunstall changed the title [WIP] [CAP-49 ] no connection serials [CAP-49 ] no connection serials Jan 6, 2023
@andydunstall andydunstall changed the title [CAP-49 ] no connection serials [CAP-49] no connection serials Jan 6, 2023
@andydunstall andydunstall marked this pull request as ready for review January 6, 2023 13:22
@SimonWoolf SimonWoolf force-pushed the feature/CAP-49-no-connection-serials branch from 89d74fe to e1c04a9 Compare January 16, 2023 12:47
@github-actions github-actions bot temporarily deployed to staging/pull/1110/typedoc January 16, 2023 12:49 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 16, 2023 12:50 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1110/bundle-report January 16, 2023 14:14 Inactive
andydunstall and others added 25 commits January 30, 2023 12:13
This is no longer needed in the new upgrade scheme.
With protocol v2 we no longer wait for a `SYNC` so this test doesn't
make sense.
…ix race

with a 12s ttl, realtime was asking the lib to start a reauth as soon as
it connected. With unlucky timings, the original token details could be
cleared just before the test code looked at it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants