delight-im / Android-DDP

[UNMAINTAINED] Meteor's Distributed Data Protocol (DDP) for clients on Android
Apache License 2.0
274 stars 54 forks source link

Clearing mQueuedMessages. #61

Closed chopikboy closed 7 years ago

chopikboy commented 8 years ago

I found 4 using of mQueuedMessages. 1) Defining field 2) Creating in constructor 3) Adding some method 4)Using for sending messages. But where we clearing this queue? If we will reconnect second time, old messages will resended again. Isnt?

ocram commented 8 years ago

Thanks for spotting this issue :)

Maybe we should test if these queued messages are really kept forever and possibly sent multiple times, but this seems to be what's happening.

Did you notice old messages being sent multiple times, or did you just find this issue when looking at the code?

Messages that are successfully sent should not be in the queue anymore and the subsequent sent call will add them again if they fail once more. So we cannot just iterate over the original collection.

What about the following fix, changing ...

for (String queuedMessage : mQueuedMessages) {

... to ...

final String[] queuedMessages = mQueuedMessages.toArray(new String[0]);
mQueuedMessages.clear();

for (String queuedMessage : queuedMessages) {

...?

joze2016 commented 7 years ago

I meet same issue, the old message being sent multiple times.

joze2016 commented 7 years ago

do you have any plan to fix this issue?

ocram commented 7 years ago

I meet same issue, the old message being sent multiple times.

Thanks! Can you verify that the actions failed and were re-sent during subsequent calls?

do you have any plan to fix this issue?

Yes.

NampyoJeong commented 7 years ago

I meet same issue.

Changing for (String queuedMessage : mQueuedMessages) { ... } to while (!mQueuedMessages.isEmpty()) { send(mQueuedMessages.poll()); } works fine.

ocram commented 7 years ago

@NampyoJeong Thanks a lot for testing!

I have now merged @PerkinsZhu’s solution from https://github.com/delight-im/Android-DDP/pull/139, which should be equivalent.

The new version of this library is v3.3.1, as shown in the updated README. Perhaps somebody can test this update as well.