Connection resumption improvements #900

ikbalkaya · 2023-01-13T21:17:54Z

This PR introduces some fixes to connection resumption issues by implementing spec RTN15c1, RTN15c2 and RTN15c3. To summarize the rule of implementation.

If connection resume is successful, current serial is set to the first serial of pending queue, queue is cleared and previously sent pending messages are added in front of queued messages.
If connection resume is failed the message serial is reset to 0 and all pending messages are added the queue to be sent with the newly assigned serials

Previously when there was a new connection established, channels were suspended and pending messages were reset. This should not be happening anymore

Also I have made some changes to general interface of WebSocketTransport to be able to mock blocking receives

There is a new receive method that uses a single method interface to delegate handling of sending messages to connectionManager. There are also some changes with MockWebSocketTransport that allows blocking incoming acks/nacks. This class is also now exposed so that we can read sent published messages directly. Please note that changes done here for these classes only serve the particular case of publishing. I think it can be improved to contain other protocol actions too.

Some changes are inspired and taken from #842

Closes #474

davyskiba

Those changes make sense, but keep in my mind my relatively small ably-java knowledge

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

paddybyers

See comments inline.

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

paddybyers

Please see inline comments.

I suggest please that we tidy this and remove the whitespace-related commits, because the commit history is now a mess.

And we still need tests pls.

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

ikbalkaya · 2023-01-19T09:28:08Z

I suggest please that we tidy this and remove the whitespace-related commits, because the commit history is now a mess.

Sorry about this, this should be clearer now - I squashed commits that were causing that

And we still need tests pls.

I have not finished addressing everything yet sorry. I should request for another review when I'm done

paddybyers

Please see inline comments

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

lib/src/test/java/io/ably/lib/test/realtime/RealtimeResumeTest.java

paddybyers

This is looking good.

Can you please tidy the commit history before merging this please?

This is the first commit that addresses connection resumption issues by picking and adapting some changes from #842 This change only apply channel reattachment and overrides the previous behaviour where pending messages were suspended if a new connection was established with a non-fatal error.

This commit makes resume case explicit by isolating it into a single if branch. There were also some comments left by Paddy which I thought will be useful to keep in codebase.

When sending messages, the reattempted messages from pending queue would be readded because sendImpl will push a new message to the queue if ack is required. This change clones previous messages from pending queue, using it as a reference to reattempt, but clears queue before reattempt. So now how it works: If the connection resumed successfully, pending messages (if available) will be added in front of queued messages, resetting the start serial to the first serial of pending messages. If connection resume is not successful, msgSerial and start serial will be reset to 0. This will also remove the explicit call to send() as the send should happen when the connection transition event arrives which should happen after this. Please note that I kept msgSerial = 0 as this will be increased in sendImpl

Message serials are now reset based on whether the connection resume succeeded or failed. In case of success, the message serial will be reset to the first serial of pending messages and in case of failure that serial will be reset to 0, the same applies to start serial in pending message queue

MockWebSocketTransport and WebSocket transport have send() but not receive() functionality, That's why we are unable to allow / block or fail incoming messages. In this commit I add a receiver interface that routes received protocol messages to websocket transport. as a result connectionManager.onMessage call moves to WebSocketTransport and as a result we can use the mock to set rules for receives. There is also a simple receive behaviour on MockWebSocketTransport that should allow us to define the same rules for receives as sends. Also MockWebSocketTransport is now public and has a new instance publishedMessages that allows us assert internals / orders of those published messages. Two tests testing the pending message behaviour : The first one is when the resume is successful and the second one is when the resume has failed

paddybyers

LGTM, thanks

jaley

I've not looked at the code but see you already had two others do that. Instead, I checked out this branch, built it and integrated it with AAT to rerun the NetworkingConnectivityTest. I can confirm that this fixes the following fault simulations:

DisconnectWithFailedResume
EnterUnresponsive

For those two, it'd be good to re-enable those tests when you integrate this release to AAT, to confirm they're green on main too.

It also fixes the problems initially causing ReenterOnResumeFailed to fail, leaving us now with just a non-fatal NACK causing an exception to be thrown - a separate known issue, so we can't re-enable that test until the fix comes it.

Nice work!

ikbalkaya changed the title ~~Reattach channel on reconnection~~ Connection resumption improvemnts Jan 13, 2023

ikbalkaya changed the title ~~Connection resumption improvemnts~~ Connection resumption improvements Jan 13, 2023

github-actions bot temporarily deployed to staging/pull/900/javadoc January 13, 2023 21:19 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 16, 2023 11:46 Inactive

ikbalkaya requested a review from paddybyers January 16, 2023 14:28

github-actions bot temporarily deployed to staging/pull/900/javadoc January 16, 2023 14:56 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 16, 2023 16:49 Inactive

ikbalkaya self-assigned this Jan 16, 2023

ikbalkaya marked this pull request as ready for review January 16, 2023 18:05

ikbalkaya requested review from QuintinWillison and davyskiba January 16, 2023 18:06

davyskiba approved these changes Jan 17, 2023

View reviewed changes

davyskiba reviewed Jan 17, 2023

View reviewed changes

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to staging/pull/900/javadoc January 17, 2023 11:04 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 17, 2023 11:06 Inactive

ikbalkaya requested a review from KacperKluka January 17, 2023 11:09

paddybyers requested changes Jan 18, 2023

View reviewed changes

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java Outdated Show resolved Hide resolved

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to staging/pull/900/javadoc January 18, 2023 16:39 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 18, 2023 16:47 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 18, 2023 17:40 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 18, 2023 17:43 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 18, 2023 17:50 Inactive

paddybyers requested changes Jan 18, 2023

View reviewed changes

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java Outdated Show resolved Hide resolved

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java Outdated Show resolved Hide resolved

ikbalkaya force-pushed the bug/lib_not_resending_pending_messages_after_resume branch from 0494dfd to 34a6826 Compare January 18, 2023 21:32

github-actions bot temporarily deployed to staging/pull/900/javadoc January 18, 2023 21:34 Inactive

ikbalkaya force-pushed the bug/lib_not_resending_pending_messages_after_resume branch from 34a6826 to d7a7081 Compare January 18, 2023 21:34

github-actions bot temporarily deployed to staging/pull/900/javadoc January 18, 2023 21:35 Inactive

ikbalkaya marked this pull request as draft January 19, 2023 11:40

github-actions bot temporarily deployed to staging/pull/900/javadoc January 19, 2023 12:19 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 20, 2023 18:41 Inactive

paddybyers requested changes Jan 23, 2023

View reviewed changes

github-actions bot temporarily deployed to staging/pull/900/javadoc January 23, 2023 14:57 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 23, 2023 15:01 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 23, 2023 15:18 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 23, 2023 15:35 Inactive

github-actions bot temporarily deployed to staging/pull/900/javadoc January 23, 2023 15:52 Inactive

ikbalkaya force-pushed the bug/lib_not_resending_pending_messages_after_resume branch from 64ce58f to a1ee4ba Compare January 23, 2023 15:55

github-actions bot temporarily deployed to staging/pull/900/javadoc January 23, 2023 15:56 Inactive

ikbalkaya marked this pull request as ready for review January 23, 2023 16:20

ikbalkaya requested review from paddybyers and jaley January 23, 2023 16:23

paddybyers reviewed Jan 24, 2023

View reviewed changes

ikbalkaya added 2 commits January 24, 2023 10:23

Make resume case explicit

c6b777f

This commit makes resume case explicit by isolating it into a single if branch. There were also some comments left by Paddy which I thought will be useful to keep in codebase.

ikbalkaya force-pushed the bug/lib_not_resending_pending_messages_after_resume branch from a1ee4ba to e46d8a5 Compare January 24, 2023 10:39

github-actions bot temporarily deployed to staging/pull/900/javadoc January 24, 2023 10:40 Inactive

ikbalkaya added 3 commits January 24, 2023 10:46

ikbalkaya force-pushed the bug/lib_not_resending_pending_messages_after_resume branch from e46d8a5 to 07ad898 Compare January 24, 2023 10:47

github-actions bot deployed to staging/pull/900/javadoc January 24, 2023 10:48 View deployment

ikbalkaya requested a review from paddybyers January 24, 2023 12:39

paddybyers approved these changes Jan 24, 2023

View reviewed changes

This was referenced Jan 24, 2023

On connect queued messages are sent before attaching channels #902

Open

Publisher can get into a bad state if offline for > 2 minutes ably/ably-asset-tracking-android#906

Closed

jaley approved these changes Jan 25, 2023

View reviewed changes

ikbalkaya merged commit 42b3608 into main Jan 25, 2023

ikbalkaya deleted the bug/lib_not_resending_pending_messages_after_resume branch January 25, 2023 13:47

jaley mentioned this pull request Jan 27, 2023

NetworkingConnectivityTest fixes ably/ably-asset-tracking-android#960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection resumption improvements #900

Connection resumption improvements #900

ikbalkaya commented Jan 13, 2023 •

edited

Loading

davyskiba left a comment

paddybyers left a comment

paddybyers left a comment

ikbalkaya commented Jan 19, 2023

paddybyers left a comment

paddybyers left a comment

paddybyers left a comment

jaley left a comment

Connection resumption improvements #900

Connection resumption improvements #900

Conversation

ikbalkaya commented Jan 13, 2023 • edited Loading

davyskiba left a comment

Choose a reason for hiding this comment

paddybyers left a comment

Choose a reason for hiding this comment

paddybyers left a comment

Choose a reason for hiding this comment

ikbalkaya commented Jan 19, 2023

paddybyers left a comment

Choose a reason for hiding this comment

paddybyers left a comment

Choose a reason for hiding this comment

paddybyers left a comment

Choose a reason for hiding this comment

jaley left a comment

Choose a reason for hiding this comment

ikbalkaya commented Jan 13, 2023 •

edited

Loading