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

[WIP] [CAP-49] no connection serials #1045

Closed
wants to merge 34 commits into from

Conversation

andydunstall
Copy link
Contributor

@andydunstall andydunstall commented Aug 19, 2022

Add support for no connection serials as described in ably/specification#88.

(Note I've updated the original PR to try to make it clearer which commits implement which spec items - so should be easier to walk though commit by commit if thats useful when implementing the other SDKs - and I've moved the ably-js specific upgrade stuff into the last two commits so those can be ignored)

Changes

  • updates the protocol version to 2.0 (RSC7a)
    • testing: covered already by initbase0
  • fix channel serial type
    • message.channelSerial is a string not a number (eg 'e02Tut8QwBFsQM05921682:5')
  • tracks channel serial of last message/presence (RTL15b)
    • note unlike with connection serials we arn't checking for duplicates by comparing serials - this was only needed since the old upgrade scheme could potentially lead to duplicates which is no longer the case, and we want to keep the channelSerial opaque rather than having to parse it to compare
    • testing: covered already by resume_active
  • includes channelSerial in ATTACH (RTL4c1)
    • testing: covered already by resume_active
  • remove connectionSerial from resume request (RTN15b2)
    • testing: covered already by resume_active
  • reattaches channels once connected
    • reattaches attached channels once connected (RTN15c6/RTN15c7)
    • attaching and suspended states aleady go though the attach sequence on connected (RTN15c6/RTN15c7)
    • no longer need to reattach all attached channels in ConnectionManager as this is now always done when we get a new connection, regardless of whether the connection id changed (RTN15c3)
    • msgSerial is already reset and errorReason is already set (RTN15c7)
    • already processes queued messages upon connection (RTN15c6/RTN15c7)
  • clears channelSerial when detached/suspended/failed (RTP5a1)
    • testing: covered by channel rewind works on channel after reattaching test (if we don't clear the rewind will fail)
  • adds recovery with channel serials
    • RTN16f is already implemented - this just updates to use the new serialised recovery key
    • loads the recovery channel serials, applies to any existing channels (which are not yet attached) and stores for later (RTN16j)
      • note currently this doesn't clear the recovery channel serials, so if the developer releases the channel then reattaches it uses the recovered channel serial again (not defined in the spec)
    • RTN16k is already implemented - this just updates to use the new serialised recovery key
    • encodes the recovery key as JSON (RTN16g)
    • RTN16d is implemented already since in those states the connectionKey is undefined
    • channel recovery keys are only loaded in connect, since the recover param may be a user defined callback it can't be loaded in the constructor, and by the time we get to connect we know theres no attached channels but recover has been resolved into a string (by getTransportParams)
    • testing: added a test for recovering multiple channels
  • presence changes
    • sends the member ID on re-entry (data and channelId already sent) (RTP17g)
    • updates presencemessage to include the id when encoding
    • re-enter whenever we become ATTACHED (RTP17f) (regardless of if HAS_PRESENCE has been set (RTP17c2))
    • no longer re-enter when we complete sync (RTP17c1)
    • when re-entering sends all members with our connectionId, regardless of whether its in the main presence map, and no longer removes from the 'my members' presence map (RTP17g/RTP17d)
    • testing: already tested by presence_auth_reenter
  • removes connection serial (RTN10/RTN15b2)
    • this breaks upgrade - which will be fixed later - though prefered to group all the spec changes together (then add the ably-js specific upgrade stuff later)
    • removes duplicateConnectionId test as now redundant
  • removes checkChannelsOnResume options
    • given we're already reattaching each channel on resume, reattaching again after 30s is redundant
  • channel doesn't wait for channelSerial if empty (RTP17h)
  • presence index mymembers by client ID (RTP17h)

Upgrade Changes
(ably-js only - from https://github.com/ably/realtime/issues/4216#issuecomment-1221407649)

  • replaces upgrade SYNC with ACTIVATE
  • doesn't wait for no channels pending before upgrading
  • removes mostRecentMessage as this is no longer needed in the new upgrade scheme (and causes a bug - see thread)
    • removes duplicateMsgId test as now redundant

@github-actions github-actions bot temporarily deployed to staging/pull/1045/bundle-report August 19, 2022 10:32 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-remove-connection-serials branch 4 times, most recently from 23de044 to 49e1f50 Compare August 23, 2022 15:31
@github-actions github-actions bot temporarily deployed to staging/pull/1045/bundle-report August 23, 2022 15:33 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-remove-connection-serials branch from 49e1f50 to ba44b8c Compare August 24, 2022 06:46
@github-actions github-actions bot temporarily deployed to staging/pull/1045/bundle-report August 24, 2022 06:48 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-remove-connection-serials branch from ba44b8c to 20eaa12 Compare August 24, 2022 07:56
@github-actions github-actions bot temporarily deployed to staging/pull/1045/bundle-report August 24, 2022 07:57 Inactive
@andydunstall andydunstall force-pushed the feature/CAP-49-remove-connection-serials branch from 45b1b73 to b5b51c1 Compare August 24, 2022 12:00
@andydunstall andydunstall force-pushed the feature/CAP-49-remove-connection-serials branch 4 times, most recently from a8fa21a to 5ae8539 Compare August 31, 2022 09:47
} catch (err) {
cb(err);
}
}, 2000);
Copy link
Collaborator

@sacOO7 sacOO7 Sep 17, 2022

Choose a reason for hiding this comment

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

Tests will become flaky because of the timeout, we can use waitforexpect instead. It will skip extra timeout ( supplied as a default moderate value, to make sure non-flaky tests) as soon as the expectation is matched against the given assertion and execute the promise callback.

Copy link
Collaborator

@sacOO7 sacOO7 Sep 17, 2022

Choose a reason for hiding this comment

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

waitForExpect(() => {
    expect(recoveredCount).to.equal(MSG_COUNT);
}).then(cb);

Copy link
Collaborator

@sacOO7 sacOO7 Sep 17, 2022

Choose a reason for hiding this comment

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

andydunstall and others added 25 commits January 4, 2023 15:42
We no longer wait for a response from the server on upgrade, so this
test is redundant.
We no longer wait for a SYNC response so the transport was being
upgraded before the channel was attached. Instead publish immediately
without waiting to attach so the message is queued while the comet
transport is still active.
With 2.0 connectionKey is only include in connectionDetails
With upgrade we don't wait for a response, so the first attach request
is ignored (as the ACTIVATE immediately suspends the channel), so the
subscriber misses the publish.
@andydunstall
Copy link
Contributor Author

Given this PR is now old and the commit history quite messy I've replaced with #1110

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