Merged
Conversation
Previously when queued messages were cleared because the states requiring them to be (when queueEvents flag was false) pending message queue wasn't effected by that This adds the functionality to clear pending queue with normal queued messages.
ikbalkaya
changed the title
ikbalkaya
commented
Jan 26, 2023
paddybyers
reviewed
Jan 27, 2023
paddybyers
reviewed
Jan 27, 2023
ikbalkaya
force-pushed
the
process_queueued_messages_after_reattach
branch
from
81a5bf1 to
6762032
Compare
ikbalkaya
force-pushed
the
process_queueued_messages_after_reattach
branch
from
6762032 to
3927bd8
Compare
ikbalkaya
force-pushed
the
process_queueued_messages_after_reattach
branch
from
2b84ca0 to
c6898b7
Compare
Message publishing on failed resume was happening prematurely before channel went into attached state, that would cause pending messages to fail. Now in case of resume failure, queued messages will be transferred to their respective channels so that reattach can happen there. Directly calling attach from the method would cause the attach to get quite late as setConnected() from connectionManager wasn't called yet. So instead I created a new flag : reattachOnConnectionResume so that I could use it to reattach when setConencted event arrived. I also removed a previous sync call from this method with recommendation of @paddybyers . Now resume tests pass but there are some channel tests that fails - and related to continuity.
ikbalkaya
force-pushed
the
process_queueued_messages_after_reattach
branch
from
c6898b7 to
76a940c
Compare
When sending reattach request to respective channel - it wasn't accounting for the case where there wouldn't be no queued messages. We need to support this so that reattach without queued messages will happen without a problem
To enable message sending while disconnected - I added a block to connect message until the message is sent so the test flakiness is no longer the case
ikbalkaya
changed the title
In case of a connection resume failure, the old keys for present members needs replacing
Replace old presence when a new connection id has arrived
paddybyers
requested changes
Feb 1, 2023
Contributor
Author
|
Moving this to draft state until tests pass / any new tests added |
According to spec RTL6c2, queued messages must be resent as soon as the connection is CONNECTED. This commit does that by removing only presence messages from existing queuedMessages and transferring them onto the channel. Also rename methods on channel and readjust to only add to pendingPresence
ikbalkaya
force-pushed
the
process_queueued_messages_after_reattach
branch
from
a03265f to
dd9a49b
Compare
As queued messages will remain on connection manager, there is no need to keep and process them on the channel level anymore
ikbalkaya
force-pushed
the
process_queueued_messages_after_reattach
branch
from
30bb018 to
0af496d
Compare
As the retry wasn't suppressed, connection could go into connecting right after disconnection - this add suppress retries and also cleans up the test Also removes unneeded logs
ikbalkaya
force-pushed
the
process_queueued_messages_after_reattach
branch
from
83047a9 to
0b05d1e
Compare
Improve this test to block detached arriving so that we are more confident that we will stay in detaching state while this is blocked
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes two issues
queueEventsis false, pending messages will also be cleared which was missing before.Previously as soon as connected message was received by connection manager, channels would directly attempt to attach. I have changed the behaviour so that after a resume failure (connected event with a new connection id) I transfer all queued messages (if any) from the connection manager to associated channel. This sets a flag for later to be processed from
setConnected. Previously when attempted to call directly to attach from reattach, that would also fail and retry would happen quite a bit later. Attach would time out and then a reattach with some delay would be invoked. I also removed a sync() call fromsetConnected()as recommended by @paddybyers .There was also an issue with previously enters members wouldn't automatically reenter. There should be a fix for this and a test that confirms the behaviour.
There are also some test and test utilities improvements.
Fixes #905
Fixes #904