search

ushahidi / SMSSync

SMS gateway for Android powered phones
http://smssync.ushahidi.com
GNU Lesser General Public License v3.0
1.14k stars 492 forks source link

Race condition in publishing can cause messages to be published multiple times #454

Open vanso-hubsi opened 8 years ago

vanso-hubsi commented 8 years ago

Expected behavior

Each message gets only published once, no matter what.

Actual behavior

With messages waiting in Incoming, each press on the publish icon seems to start a publishing thread, even if it is already running.

When there's an issue with the internet or the receiving web service, that makes requests from SMSSync time out, each publishing request takes 30 seconds to time out, making the publishing thread take a long time. Say you have 20 messages in Incoming due to connection issues and you press the publish button, those 20 messages are picked by a new publishing thread and each attempt to deliver takes 30 seconds. If you press the publish button again after one minute (thinking it didn't work), another publishing thread is started in parallel, trying to submit the very same messages again. If the connection starts working again while those threads are still running, the remaining messages in both tasks get sent to the server, resulting in messages being delivered twice. And if the auto sync job stepped in, they'd even be delivered three times.

The same happens if you accidently hit the publish button multiple times.

Steps to reproduce the behavior

see above

Notes

I hope my analysis is correct. If so, the following solutions I'd like to suggest come to my mind:

  1. disable "manual" publishing as long as it's running. I think at the moment, the loading icon is only displayed until the first of the pending messages has been processed, and the publish button is not disabled at all. I'd suggest to hide the publish button and keep the loading icon until the thread has finished. Unfortunately, this doesn't prevent the auto sync job from stepping in.
  2. before allowing any publishing job (manual and auto sync) to run, check if one of both is already running. Still, the publish button should be hidden and the loading icon kept until finished.
  3. change the design: in both jobs process messages one by one, taking them "out" of the queue (either by moving a message to RAM or better by setting it to a processing status). This change might take too much effort, though.
gnguyo commented 8 years ago

Any fix for this yet?

levimatheri commented 8 years ago

I'd suggest using a thread pool where a thread is only called if it needs to be used.