[FIX] Messages rendering out of order bug

[Shailesh351]

@RocketChat/android

Closes #1677
Closes #1926

Changes:

• When sending new msg set timestamp to max of ((timestamp of last msg)+1 and current time of local system)

• When updated msg comes check its timestamp. If timestamp of msg is greater than next msg than update its postion

[gigawhitlocks]

I'm not sure this is a complete fix for this bug. This part in particular worries me:

When sending new msg set timestamp to max of ((timestamp of last msg)+1 and current time of local system)

How is the behavior affected if the user's phone is in the future compared to the server?

Time drift problems like this one occur when a canonical time source is not agreed upon. At no point in the rendering of the chat box should the client time source be trusted. When a message is sent from the client, these are the steps that should be followed:

1. Send the message to the server, and display the message greyed out, to indicate that it is sending (this behavior exists today)
2. Wait until the server has processed the incoming message by waiting on the incoming stream of messages until the one the client just sent is sent back on the same channel where other users' messages come in
3. Destroy the greyed out message and render the message as it was returned from the server so that the server's canonical version of the message is rendered for the user. This way if anything happens -- time drift, server mangling of the message, anything -- in the interim, the user of the Android client sees the same version of the message in the same order as the other participants in the channel. This part is critical.

This issue is caused by an underlying architectural error: rendering the sent version of the message as though it is the canonical one agreed upon by the server, rather than waiting and rendering a version of the message which has been processed upstream.

[geekgonecrazy]

So seems like this should solve.

When sending new msg set timestamp to max of ((timestamp of last msg)+1 and current time of local system)

Ensures that the message will never appear above the message that just came in right before I sent. This would be the greyed out state this is forcing it to the very bottom.

When updated msg comes check its timestamp. If timestamp of msg is greater than next msg than update its postion

This then would ensure integrity and makes sure that the message has the true trusted server side timestamp for the final rendered non-sending state.

This chunk does as suggested in 3.

                    // Position of new sent message is wrong because of device local time is behind server time
                    with(chatRoomAdapter) {
                        removeItem(message.last().messageId)
                        prependData(listOf(message.last()))
                        notifyDataSetChanged()
                    }

[Shailesh351]

@gigawhitlocks Three steps you mentioned are already there. The rendering issue was there because of some bug in android implementation.

1. Because of using only local timestamp it puts msg in between previous msgs. And may be if time difference is more than user can even see grayed msg.
   So it is better to make timestamp based on last msg in the room if clock is behind from server.

How is the behavior affected if the user's phone is in the future compared to the server?

It will put new msg at bottom so it works like a charm.

2. We use recycler view for showing list of messages and because of bug it was not updating msg with server updated timestamp. So for positioning it correctly I added one conditional check.
   dataSet[ind].message.timestamp > dataSet[ind - 1].message.timestamp)

If timestamp of current msg is less than next msg then its position is wrong. So I am removing it from there and prepending it at start.

Hope you got the fix.

Thanks for your review 😄

[gigawhitlocks]

Thank you!

[for7raid]

Is it possible to change 0,1,2 to Enum type in Kotlin? will be more readable core

[gigawhitlocks]

It will put new msg at bottom so it works like a charm.

Reading this change again I'm still concerned that if the server time is behind the user's phone time, rather than displaying the messages in the correct order, the user's message will get "stuck" to the bottom of the display until the time difference is resolved. New messages arriving, timestamped by the server in "the past", would get inserted above the user's message.

This does resolve problems when the user's phone is behind the server, though.

[philipbrito]

@Shailesh351 Thank you for submitting this PR. A small improvement should be done by indicating us what 0,1 and 2 means for you code. I've added some FIXME comments there though (would be very good if you could improve it). Thank you! 🚀

[philipbrito]

What's 0,1 and 2 means for here?

[philipbrito]

What's 0,1 and 2 means for here?

[gigawhitlocks]

Why was this merged without the review comments being addressed? Was any testing performed to show that this "solution" doesn't cause messages to stick to the bottom of the display on the mobile client if the server time drifts into the past?

What a waste of a $50 bounty!
Where are the tests to show this resolves the issue and to prevent future regression?

[Shailesh351]

@gigawhitlocks

Reading this change again I'm still concerned that if the server time is behind the user's phone time, rather than displaying the messages in the correct order, the user's message will get "stuck" to the bottom of the display until the time difference is resolved. New messages arriving, timestamped by the server in "the past", would get inserted above the user's message.

Appending new message at last is the natural behaviour expected in all the chat application. So when you send msg it just gets appended to msg list of chat. Its not like, that msg will get stuck at bottom.

If you send multiple msg then it appends msg one by one at the end. and when these msgs get back from server with corrected timestamp they are updated at same sequence so it will maintain the order of the msg sent.

max of ((timestamp of last msg)+1 and current time of local system) It ensures that new msg will be at the bottom of previous msgs.

So when server time drifts into the past then still msgs will be in the correct order. You can test this by just changing the system time in future.
Thank you.

[Shailesh351]

@filipedelimabrito @for7raid I will surely update that 0, 1, 2 implementation. Thanks for review.

[Shailesh351]

@gigawhitlocks I just tested it again. It is working. Sharing the screenshot here. Notice the local time and the time when msg is received from server with correct timestamp. Server is behind.

[gigawhitlocks]

Thank you for the additional testing, I'll make sure the bounty goes through

[philipbrito]

@gigawhitlocks

Why was this merged without the review comments being addressed?

Because it doesn't break the bug fix done here. Because it is only an improvement on the code. Because we added some FIXME comments on the code in order to improve it on the future - and it is just a small code improvement. Because we released an app version yesterday and we wanted to include this fix on it (otherwise you - actually all users - would expect more time to see it on Play Store).

What a waste of a $50 bounty!

No, you don't wasted $50. It fixes the bug (from all the test done here) - as proven now by @Shailesh351 above. You are also able to release the bounty or not.

Hope it clears your concerns here.
Thank you.