Fix a rare issue with incorrect message order when sending messages offline #3316
Conversation
This file contains 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
laevandus
added
🐞 Bug
Generated by 🚫 Danger |
laevandus
force-pushed
the
fix/incorrect-message-order-while-offline
branch
from
85cee3b
to
a05ad79
Compare
Sources/StreamChat/APIClient/Endpoints/EndpointPath+OfflineRequest.swift
Outdated
Show resolved
Hide resolved
laevandus
force-pushed
the
fix/incorrect-message-order-while-offline
branch
from
a05ad79
to
f058c43
Compare
laevandus
force-pushed
the
fix/incorrect-message-order-while-offline
branch
from
f058c43
to
ed70b02
Compare
|
After a longer discussion we'll change the direction a bit. |
laevandus
changed the title
…essages while offline
laevandus
force-pushed
the
fix/incorrect-message-order-while-offline
branch
from
ed70b02
to
45f360d
Compare
laevandus
changed the title
StreamChat XCMetrics
|
nuno-vieira
reviewed
Jul 24, 2024
nuno-vieira
requested changes
Jul 24, 2024
laevandus
commented
Jul 26, 2024
| @@ -88,6 +88,47 @@ class MessageRepository { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| /// Marks the message's local status to failed and adds it to the offline retry which sends the message when connection comes back. | |||
| func scheduleOfflineRetry(for messageId: MessageId, completion: @escaping (Result<ChatMessage, MessageRepositoryError>) -> Void) { | |||
There was a problem hiding this comment.
MessageSender uses this to send queued messages directly to offline repo when it sees that there is connection issue.
laevandus
commented
Jul 26, 2024
Comment on lines
+135
to
+139
| // Run offline actions requests as the first thing | ||
| if config.isLocalStorageEnabled { | ||
| operations.append(ExecutePendingOfflineActions(offlineRequestsRepository: offlineRequestsRepository)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Moved it to the first item on the syncing list so that we retry more quickly rather than waiting after other stuff.
|
martinmitrevski
approved these changes
Jul 26, 2024
nuno-vieira
added
the
❗️ High Priority
testableapple
added
🟢 QAed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🔗 Issue Links
Resolves PBE-4319
🎯 Goal
Fix the issue of incorrect order of messages when multiple messages are in the pending send state and connection comes back.
📝 Summary
OfflineRequestsRepositoryfor retrying on the next web-socket connect event (otherwise we have a race between it andMessageSender). Message statuses are set to.sendingFailed.MessageSenderis waiting for connection, it sends all the requests to the offline repository until it is notified connection status change (ensures thatMessageSenderandOfflineRequestsRepositoryare not sending messages for the came channel at the same time).OfflineRequestsRepositoryremoves duplicate send message requests (in case or manual retries while offline)🛠 Implementation
APIClientsends failed API requests toSyncRepositoryhere. This will then be passed toOfflineRequestsRepositorywhich uses the shouldBeQueuedOffline for deciding if it should retry the failed request.When this happens, we have
MessageSenderandOfflineRequestsRepositoryboth trying to send messages on connect. This PR eliminates this race and makesOfflineRequestsRepositoryto be solely responsible of sending messages when connection status changes. When we have connected,MessageSenderis allowed to send messages again if user does so. But all the previous ones, are handled byOfflineRequestsRepository.🎨 Showcase
Recorded from the point where the first request has failed with a connection issue.
🧪 Manual Testing Notes
Test case 1:
Test case 2
Messages are sent one by one. Currently order changes because server gives updated timestamps. Can be improved if we would like to, but I did not want to include more changes to this PR. Solution would be to update local timestamps after each message send.
☑️ Contributor Checklist