Cloudkibo / Android

0 stars 0 forks source link

Message Order disturbed when received pending messages from iOS #561

Closed sumairasaeed closed 7 years ago

sumairasaeed commented 7 years ago

Repro steps: Android is on conversation window and ONLINE Switch OFF internet on iOS Type and sending multiple message from iOS to Android Switch ON internet on iOS Android receives messages but in wrong order

jekram commented 7 years ago

@sumairasaeed

Why there is no screen shot? we should have screen shot of sender and the destination device so people can debug it?

Please repeat and provide the above information.

sumairasaeed commented 7 years ago

I have edited the description of problem and added repro-steps. Tested again and found that this error is reproduced both when sending messages from iOS to Android and Android to iOS .

My analysis is that this is a server side issue as the problem is observed both on Android and iOS. From my analysis of problem, the cause of error is that when we do upward sync, server is sending message push notifications in incorrect order. @sojharo can further comment on it as he worked on server-side logic for upward sync.

Here are screenshots: image


image

jekram commented 7 years ago

Is there not a time stamp that we should be using?

jekram commented 7 years ago

@sumairasaeed I thought we went to process to put the time stamp and we were going to use that on the clinet so I do not know why do you think this is a server issue

sumairasaeed commented 7 years ago

Sir as we have modified our sync logic, now we are send all pending messages at the same time once in a form of array to server. The timestamp used is the zulo/UTC time as agreed on previously.

jekram commented 7 years ago

I do not understand your comment. Are you saying that message a, b, c will have the time stamp from the sender? then how do you expect the server to fix this problem or the receiver to fix the problem?

jekram commented 7 years ago

I do not understand your comment. Are you saying that message a, b, c will have the same time stamp from the sender? then how do you expect the server to fix this problem or the receiver to fix the problem?

sojharo commented 7 years ago

I have debugged it and we can solve it by timestamp. We should not rely on the order push notifications are received. The server has asynchronous calls during upward sync. They are undeterministic and push will not go in order. The client should display the messages in order. I had seen once whatsapp doing this. I got two messages in incorrect order on whatsapp but quickly within a second they automatically ordered themselves.

I have debugged here in this issue. I am not fixing it now as it is medium priority. I would fix it later on android side to order the messages quickly as they arrive.

sumairasaeed commented 7 years ago

We can handle it on chat UI by reordering using timestamp. But the push received will still be in incorrect order as @sojharo you have mentioned that server is sending pushs as asynchronous calls and thus order is not maintained. So if phone is locked, pushs are displayed as received and can be in any order. That cannot be handled on client side. correct? Yes we can handle it on UI when app is opened. We can reorder there. Agreed.

On Tue, Mar 28, 2017 at 11:55 AM, Sojharo notifications@github.com wrote:

I have debugged it and we can solve it by timestamp. We should not rely on the order push notifications are received. The server has asynchronous calls during upward sync. They are undeterministic and push will not go in order. The client should display the messages in order. I had seen once whatsapp doing this. I got two messages in incorrect order on whatsapp but quickly within a second they automatically ordered themselves.

I have debugged here in this issue. I am not fixing it now as it is medium priority. I would fix it later on android side to order the messages quickly as they arrive.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Cloudkibo/Android/issues/561#issuecomment-289680529, or mute the thread https://github.com/notifications/unsubscribe-auth/AKbhp-EK6up0z8PxeI9CBRsV2au6ho_hks5rqK7hgaJpZM4MpmSb .

jekram commented 7 years ago

@sumairasaeed You are incorrect, and it is not a server issue.

Please read @sojharo comment. It needs to be fixed on the client. What is the purpose of time stamp?

jekram commented 7 years ago

@sojharo

If we have the payload from the clinet in order. Why we cannot send the notifications in order also?

When messages are arrived out of order on the clinet we can sort it but it will be better to have the notifications go out in order. Let me know?

sojharo commented 7 years ago

It is due to asynchronous callbacks. In our previous link for each unsent message (pending message) client sent separate http request one by one to server. And server sent the message to receiver as it got. With new sync logic, the client just sends one http request which contains array of messages that need to be sent to mobile.

Now, server does this with that array:

  1. run async loop traversal to get each message in array
  2. when message is got, it checks the receiver and see if he is not blocked (async task, it checks in mongodb)
  3. after this, it sends the message to push notification service (azure, which will then schedule the message to send to client)
  4. the message is then saved to mongodb

As all these steps are asynchronous, so they are callback at different times. Moreover, azure also schedules sending of messages.

As these 3 steps are asynchronous and needs to be checked before sending the message, therefore the callbacks of these are unpredicted. It is because database call (mongodb) is happening for all messages before sending them.

We should therefore handle it on client side as our timestamp is not getting disturbed anywhere.

sojharo commented 7 years ago

We can send the notifications in order. And send them first before all other work is done on server. It has a risk. If notification reaches client and clients asks for the message from server before it is saved on mongodb then client will not get the message. We can do trial run of this logic but I am not very much confident with this.

jekram commented 7 years ago

I have couple of questions:

  1. On the server you say we run Steps 1 thur Step 3 asynchronously and then we write it to MangoDB. Why we would not write it to DB before doing these three steps. Then in no case notification will retrive a message that is not filed by the server.

  2. In reading the design document it looks like the server responds back to the client with notification. Why are we using notification in this case? In a normal case when we do a Rest API call we do not use notification. Should we not use the same logic here?

sojharo commented 7 years ago

For point 1. We must check from the mongodb if the contact is blocked or not. If the contact is blocked then we should not send the push notification. This needs to be checked for iOS. Because Android can handle the push notification if it comes for the message of blocked contact and android can avoid showing message alert and chime. But iOS can't avoid this. so server needs to check if the contact is blocked or not.

For point 2. This is the upward sync for the person who is trying to send the messages which were not sent due to no internet. When the messages reach server using upward sync. Server needs to inform the receiver by push notification that there is a message to fetch. If the receivers are online then would fetch the message on getting push notification. If receivers are offline, then they would do the downward sync using http rest api later when they come online.

jekram commented 7 years ago

@sojharo

  1. So we can do can do # 2 and # 4 and then do #3

  2. I think you missed understood my question. I was asking why we need to send Push Notification to the Sender. Since we are going Rest Call to send the and if get success or failure why do we need notification in this case.

sojharo commented 7 years ago

For point 2. Yes I had misunderstood the question and sumaira and I had a detailed discussion on this point yesterday. We agreed that in response of upward sync http request, we should only status as success or failure and not send push notifications. The push notifications payload sometimes increase the payload limit size. Sumaira has opened a bug for this on server side. While fixing that, I would also fix and avoid sending push in response to http request.

jekram commented 7 years ago

Thanks