ably-js icon indicating copy to clipboard operation
ably-js copied to clipboard

[WIP] [CAP-49] no connection serials

Open andydunstall opened this issue 3 years ago • 1 comments

(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

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

Broken Tests Listing the tests that are currently breaking due to missing realtime changes. Excluding tests failing due to missing upgrade in realtime.

  • connection - connectionQueuing
    • fails with Invalid connection serial since we no longer send a serial - will be fixed by the realtime changes
  • resume - channel_resumed_flag
    • fails with Invalid connection serial since we no longer send a serial - will be fixed by the realtime changes
  • presence - presence_auto_reenter
    • failing since we get a new connection due to Invalid connection serial - fixed by realtime changes

andydunstall avatar Aug 19 '22 10:08 andydunstall

Thanks for adding new test, it's more readable now : )

sacOO7 avatar Sep 23 '22 05:09 sacOO7

Can we add spec id as a comment next to code onwards ... it's really hard to refer to the code and relevant spec item when looking at the PR

sacOO7 avatar Sep 26 '22 13:09 sacOO7

Can we add spec id as a comment next to code onwards ... it's really hard to refer to the code and relevant spec item when looking at the PR

Each commit is labeled with the spec ID - so probably best to walk though commit by commit

andydunstall avatar Sep 27 '22 12:09 andydunstall

We will be merging this PR to integration-2.0. rather than main right?

sacOO7 avatar Oct 11 '22 18:10 sacOO7

Given this PR is now old and the commit history quite messy I've replaced with https://github.com/ably/ably-js/pull/1110

andydunstall avatar Jan 06 '23 13:01 andydunstall