Protocol 2.0 #842
Protocol 2.0 #842
Conversation
Update tests for version check
Make msgSerial public in ConnectionManager Add ConnectionRecoveryKey class for generating key json Add channelSerial in RealtimeChannelBase for generating recovery key Fix tests implementation for new recovery key spec
Implement channelSerial to attach message by spec RTL4c1
… is replaced by channelSerial
Add try/catch for generating class from json in ConnectionRecoveryKey
Add resending pending messages by spec RTN19a
There was a problem hiding this comment.
Even though I said that I don't feel confident in leaving an approval I have to do it because Github ignores my "Comment" and shows my review as "Request changes" 😅 Therefore, in order to not block this PR I'm leaving an approval 👍
| PresenceMessage member = entry.getValue(); | ||
| member.action = PresenceMessage.Action.enter; | ||
| try { | ||
| updatePresence(member, null); |
There was a problem hiding this comment.
- Could you elaborate on why you replaced
PresenceMessage itemToSend = new PresenceMessage();
itemToSend.clientId = item.clientId;
itemToSend.data = item.data;
itemToSend.action = PresenceMessage.Action.enter;
with
PresenceMessage member = entry.getValue();
member.action = PresenceMessage.Action.enter;
It's not obvious that that change is correct (won't it have a possibly-stale connectionId?)
- Why did you remove the completionListener that was previously being passed to updatePresence? Is there some magic that will turn an async failure into an exception if a completionListener isn't passed that I'm missing?
There was a problem hiding this comment.
I guess I misinterpreted RTP17g.
Is this a good implementation 28531da?
# Conflicts: # android/src/main/java/io/ably/lib/push/LocalDevice.java # core/src/main/java/io/ably/lib/realtime/Connection.java # core/src/main/java/io/ably/lib/rest/DeviceDetails.java # core/src/main/java/io/ably/lib/types/ChannelProperties.java # core/src/main/java/io/ably/lib/types/PresenceMessage.java
|
I'm moving this pull request back to Draft state because it's currently based on top of the wrong branch ( Additionally we can't upgrade this SDK to utilise protocol version |
QuintinWillison
removed request for
SimonWoolf,
QuintinWillison and
andydunstall
Given that protocol v2 completely changes the presence-auto-re-enter behaviour anyway, and also that we're going to be releasing protocol v2 as a patch to the current version, I'd argue that it doesn't make sense to do #859 first, we should just focus on protocol v2 |
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. Building library in my local I observed this particular commit will increase the number of passed tests by one (previously 11 was passing out of 18, now 12)
This commit adds remaining changes (excluding tests from #842 Changes are received as is and only some name refactorings were made. You should check the linked PR to see individual commits. Tests will be added in subsequent PRs
| if (pendingMessages.queue != null && !pendingMessages.queue.isEmpty()) { | ||
| for (final QueuedMessage queuedMessage : pendingMessages.queue) { | ||
| try { | ||
| send(queuedMessage.msg, false, null); |
There was a problem hiding this comment.
Completion listener is ignored here
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.
|
Closing in favor of #842 |
No description provided.