-
Notifications
You must be signed in to change notification settings - Fork 195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Failing to update localMessageState
results unwanted message reordering
#2693
Comments
cc @jfru |
Hi @chan-reclip, Thank you for the report. We'll have a look into it. Couple of comments: Which evidence do you have that support that you reach MessageRepository L57 with an error? Is it logs or is it an assumption in this case? Just asking because I am not sure based on the comment if you saw that happening or not. I think this would be the most important thing for us to try to help you 😄 Regardless of the previous:
If the write fails, chances are that either the message does not exist, that it has been deleted, or that the DB is in a bad state. I am mostly inclined towards the latest as there is no throwing function that is called in that process that could justify an error being reported in L57.
Based on the assumption that I stated above, if seems "safe" to assume that if the message was not retrieved in the previous step, there is no way to recover from it. So this is the reason why we are ignoring the error, and removing the message from the queue. In any case, we are going to have a deeper look into it whenever possible. |
Hi @polqf, Thanks for the prompt response.
Totally agreed but unfortunately I couldn't get any logs from the original reporter. 😞
Yes, I also think the message should exist as we check it just before the write.
It's "safe" if message was not retrieved. But haven't you said you're mostly inclined to DB being in a bad state?
I'll be waiting for it. Cheers! |
NB: This bug report is based on my speculation and I don't have repro steps or a concrete proof that what I described below is exactly what happened.
We've recently encountered this issue again, so I had a look at the code to debug possible reasons.
While browsing through the call stack, I found a suspicious code in MessageRepository.swift.
For whatever reason,
database.write
(L53) fails, we will get into the error handling code (L58-61) which basically callsmarkMessageAsFailedToSend
to updatelocalMessageState
to.sendingFailed
.stream-chat-swift/Sources/StreamChat/Repositories/MessageRepository.swift
Lines 52 to 63 in cd24f3f
I see two issues here:
localMessageState
works only when the old state is.sending
. Given we failed to update the state to.sending
, it won't work.stream-chat-swift/Sources/StreamChat/Repositories/MessageRepository.swift
Lines 130 to 132 in cd24f3f
.sendingFailed
because that's the samedatabase.write
operation which just failed.Anyways, regardless of this issue,
MessageSendingQueue
will remove the request once it's tried.stream-chat-swift/Sources/StreamChat/Workers/Background/MessageSender.swift
Lines 146 to 148 in cd24f3f
The above code ignores the result
_
because, I believe, it assumesendMessage
will either succeed or updatelocalMessageState
to.sendingFailed
. Unfortunately, that assumption seems to be wrong.If there are more requests in the send request queue, those requests could hit the backend earlier than the above failed request. Because the failed message's
localMessageState
will stay as.pendingSend
, it will be retried later. This will eventually result message delaying and thus reordering.The text was updated successfully, but these errors were encountered: