stream-chat-swift icon indicating copy to clipboard operation
stream-chat-swift copied to clipboard

Failing to update `localMessageState` results unwanted message reordering

Open chan-reclip opened this issue 1 year ago • 3 comments

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 calls markMessageAsFailedToSend to update localMessageState to .sendingFailed. https://github.com/GetStream/stream-chat-swift/blob/cd24f3fdd40f9dea881830f56ce23ff9186ba802/Sources/StreamChat/Repositories/MessageRepository.swift#L52-L63

I see two issues here:

  1. The logic to update localMessageState works only when the old state is .sending. Given we failed to update the state to .sending, it won't work. https://github.com/GetStream/stream-chat-swift/blob/cd24f3fdd40f9dea881830f56ce23ff9186ba802/Sources/StreamChat/Repositories/MessageRepository.swift#L130-L132
  2. Even if the old state checking issue is somehow fixed, I'm not too sure it can successfully update the state to .sendingFailed because that's the same database.write operation which just failed.

Anyways, regardless of this issue, MessageSendingQueue will remove the request once it's tried. https://github.com/GetStream/stream-chat-swift/blob/cd24f3fdd40f9dea881830f56ce23ff9186ba802/Sources/StreamChat/Workers/Background/MessageSender.swift#L146-L148 The above code ignores the result _ because, I believe, it assume sendMessage will either succeed or update localMessageState 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.

chan-reclip avatar Jul 05 '23 05:07 chan-reclip