Repository navigation
Lib is not re-sending pending messages on new transport after a resume #474
Description
Log posted by a customer here (cf conversation) shows several issues:
- At one point it disconnects the transport for inactivity at 14:29:08.086Z, 16s after getting a detached from realtime at 14:28:52.151. That shouldn't happen.
- After it reconnects and resumes, it does not seem to be re-sending messages on the new transport that were pending on the previous transport, as required by RTN15f
- Related to the previous one: messages called back with a failure due to getting an ack for a higher msgSerial than the pending queue are called back with "Unknown error / 50000 / 500", which could be more informative
Activity
paddybyers commentedon Oct 30, 2019
The customer log contains some sensitive information so we're not able to attach it here.
However, I suggest that you start by creating a test for https://docs.ably.io/client-lib-development-guide/features/#RTN15f and see what happens, specifically relating to sending pending messages on a reestablished transport.
amihaiemil commentedon Oct 30, 2019
@paddybyers I did a little research and I landed on RealTimeResumeTest -- as far as I understand, this is where we should add the test you proposed. Am I right, or at least close to it? :D
amihaiemil commentedon Oct 31, 2019
@SimonWoolf This ticket is rather old, but maybe you remember about it. I'm new to the project and currently trying to figure out the architecture/concepts etc. From what I understand, by "transport" you actually mean "channel", right?
paddybyers commentedon Oct 31, 2019
@SimonWoolf btw we discussed ^
added test for ably#474
ably#474 Code Review changes
added test for #474
#474 Code Review changes
paddybyers commentedon Jan 12, 2023
The bug is here, clear as day: https://github.com/ably/ably-java/blob/main/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java#L1209
Instead, queued messages should be reattempted, as per https://sdk.ably.com/builds/ably/specification/main/features/#RTN15c7
The current behaviour originates from this commit: 3c7b2d0
ikbalkaya commentedon Jan 12, 2023
@paddybyers If I am not mistaken, that particular code block refers to
pendingMessageswhile spec talks aboutqueuedMessageswhich is a different one. I'm trying to confirm that those queued messages are published after reconnection but I wanted to check whether I understand that point correctly.paddybyers commentedon Jan 13, 2023
There's a further problem with the handling of the failed resume case. The spec (https://sdk.ably.com/builds/ably/specification/main/features/#RTN15c7) says that existing channels transition to
ATTACHING(and init a new attach sequence) if there is a new connection established. However, this library is transitioning all channels toSUSPENDED(https://sdk.ably.com/builds/ably/specification/main/features/#RTN15c7). This will also prevent pending messages from being reattempted after connection.paddybyers commentedon Jan 13, 2023
There are two mechanisms implemented in ably-java.
ClientOptions.queueMessages == true.ConnectionManagerthat have been sent and are awaiting acknowledgement.These are to meet two different requirements.
I do think the spec is potentially confusing, but see https://sdk.ably.com/builds/ably/specification/main/features/#RTN19a1 - using the same queue is an implementation option, but ably-java does not do this.
Nonetheless, the requirement is that messages in
ConnectionManager.pendingMessagesneed to be reattempted on reconnection (whether the resume succeeded or not).SimonWoolf commentedon Jan 13, 2023
Note that this bug was fixed as part of the protocol v2 work, see https://github.com/ably/ably-java/pull/842/files#diff-dd49abde9c821fd279fba13b6207618933be4a96a7625f0378160b2187853de9R1510-R1530
ikbalkaya commentedon Jan 13, 2023
Thank you @SimonWoolf. I think we can use those changes to fix issues related to connection resumption here but we cannot directly use that branch. Since those changes target a different branch, which has a different project structure and has breaking changes in it we won't be able to merge this in the near future.
But I am thinking of picking of changes from there and apply it to another branch that targets the main branch and then run @jaley's tests against those changes. I think this will probably be the quickest way to fix those resumption related issues without doing much work. WDYT? cc @paddybyers
SimonWoolf commentedon Jan 13, 2023
It shouldn't, base should be changed to main per ably/specification#119
What do you mean by this?
it has one breaking change, which per #868 should be made non-breaking before we merge it as a patch release
ikbalkaya commentedon Jan 13, 2023
Before connection resumption / recovery changes there was another work to improve project structure. See #767 and Aka-protocol-2.0 branch is based on that. If I'm not mistaken the project restructure would cause breaking changes (class names, visibility etc) @QuintinWillison might shine more light on that but considering both PRs are on draft state, I do not think we can merge any of them soon.
And yes, I'm aware of that spec change which made changes non-breaking but in java case I think the base branch will be breaking
paddybyers commentedon Jan 13, 2023
@ikbalkaya I think using that branch as inspiration, and perhaps cherry-picking some of the code, would be a sensible way forward here
ikbalkaya commentedon Jan 16, 2023
@paddybyers As I mentioned on the meeting that (https://sdk.ably.com/builds/ably/specification/main/features/#RTN15c7) point was added recently with protocol 2.0
See : https://github.com/ably/specification/pull/88/files
So instead I'm going to look into making the changes for older spec point (RTN15c2 ) but will just keep anything that fixes this particular issue without any additional features.
ikbalkaya commentedon Jan 16, 2023
@SimonWoolf Sorry if that's not a question for you but I am a bit confused by this old spec point RTN15c3
My question is that what's the difference between a new attach or an attach? Isn't this going to be the same operation for three states (
channel.attach()) Or is it just a wording that states the fact that previously attached channels will be attached again?SimonWoolf commentedon Jan 18, 2023
Yeah, that one was very poorly worded. That's why I rewrote it 🙂
What it was trying to say was that if you're
suspendedyou should start a normal attach (move to theattachingstate and send an ATTACH), and if you're inattachingorattachedyou should send an ATTACH but not otherwise change state.In the new spec, this becomes even simpler (
The client library should move all channels that were in the @ATTACHING@, @ATTACHED@, or @SUSPENDED@ states to the @ATTACHING@ state, and initiate an @RTL4c@ attach sequence for each.).(Again, this is all fixed in protocol-2. Is it really so unfeasable to rebase that branch on
mainso it can be released without breaking changes?)ikbalkaya commentedon Jan 18, 2023
Not really, I actually did that (without adding tests - but tests could also be added easily).
Commit with other changes added from protocol-2.0 branch (that needs protocol version be 2.0 not 1.3) I remember @paddybyers mentioning this being a big change on standup and was not very keen, but I think given that it actually resolves some existing issues with also using the new protocol it might be worth the change but I cannot know if new issues will emerge as a result.
My concern was that that was still in draft state even if there are approvals. If other people are ok I can progress with bringing all changes from there.
I'm also now doing some experiments on whether changes will help some AAT tests pass : ably/ably-asset-tracking-android@99f5b68
paddybyers commentedon Jan 18, 2023
We have two branches: protocol 1.x and protocol 2.0.
For protocol 1.x we're fixing bugs, but preserving 1.x behaviour.
For the protocol 2.0 branch, we're getting it to the point that it's stable and ready for release.
Please let's not confuse those two things.