fix: block flush tasks from running simultaneously

[wzieba]

Closes: #65

By introducing flushInProgress flag, we limit the possibility of executing multiple FlushQueue AsyncTasks simultaneously, which could result in sending the same payload multiple times.

Reproduce the issue

1. Checkout current master
2. Apply PATCH

PATCH

Subject: [PATCH] refactor: move plugins configuration to pluginManagement
---
Index: example/src/main/java/com/example/MainActivity.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/example/src/main/java/com/example/MainActivity.java b/example/src/main/java/com/example/MainActivity.java
--- a/example/src/main/java/com/example/MainActivity.java	(revision 74acc5131530cc511155691ffe39c26c23d1ffd3)
+++ b/example/src/main/java/com/example/MainActivity.java	(date 1694441142553)
@@ -26,7 +26,7 @@
         ParselyTracker.sharedInstance("example.com", 30, this);
 
         // Set debugging to true so we don't actually send things to Parse.ly
-        ParselyTracker.sharedInstance().setDebug(true);
+        ParselyTracker.sharedInstance().setDebug(false);
 
         final TextView queueView = (TextView) findViewById(R.id.queue_size);
         queueView.setText(String.format("Queued events: %d", ParselyTracker.sharedInstance().queueSize()));
@@ -147,5 +147,8 @@
 
     public void trackReset(View view) {ParselyTracker.sharedInstance().resetVideo(); }
 
-    public void flushEventQueue(View view) {ParselyTracker.sharedInstance().flushEventQueue(); }
+    public void flushEventQueue(View view) {
+        ParselyTracker.sharedInstance().flushEventQueue();
+        ParselyTracker.sharedInstance().flushEventQueue();
+    }
 }

3. Run the example app
4. Click on Track URL
5. Tap on Flush event queue
6. Assert that you can see [Parsely] POST Data (...) log twice

Test the fix

Perform the same steps as above, but checkout this branch (issue/65_do_not_send_multiple_requests) and at step 6 assert that you can see only one [Parsely] POST Data (...) log.

[ParaskP7]

👋 @wzieba !

I have reviewed and tested this PR as per the instructions, it didn't works as expected, please check my blocker (🚫) comment! 🤔

---

Furthermore, I wonder if introducing yet another state (flushInProgress) here is the best we could do, and more so, if that wouldn't introduce some kind of another bug (can't think of any, that is, out of the top of my head, but still). Wdyt? 🤔

What worries me is if there are other valid scenarios where flushEventQueue() needs to be triggered twice (or more), while the previous doInBackground(...) is still in-progress. Again, can't think of anything, and I just want to open this discussion now so that you could maybe investigate this and make sure that we are 100% sure that as we are fixing a potential bug, we are not introducing another regression of some kind. 🤔

Also, I wonder why the designers of ParselyTracker.java didn't think about that case, guarding against multiple flushEventQueue() calls, as it seems pretty easy to do. Maybe they did, but decided against it, maybe not. Or maybe, they didn't think of that because trying multiple flushEventQueue() calls, the way we tested it, via the FLUSH EVENT QUEUE is something that can't/shouldn't be done manually in production apps using our SKD, and instead, only depend on the timer to only do that automatically, based on the flushInterval parameter, after a specific period of time. 🤔

Me keep writing and thinking about it, I wonder whether it would make more sense to somehow limit the flushEventQueue() to be an internal or private method, only accessed by the timer (see FlushManager.runningTask), and also, maybe even limit the flushInterval to a minimum allowed. For example, currently, entering a value of 0 seconds is allowed, but this would cause an IllegalArgumentException exception for a Non-positive period., but maybe we should even disallow/overwrite 0-4 seconds, setting the minimum to 5 in order to avoid sequential flushes very close to each other.

Apologies if all the above are not helping or even irrelevant, just wanted to share as much of my thoughts here just in case they could help you move forward. 😊

[ParaskP7]

Blocker (🚫): Unfortunately, this wouldn't work as is (tested too). What you need is to set flushInProgress to true right after this if (!flushInProgress) check and remove the flushInProgress = true; at the beginning of doInBackground(...) (not needed), only leaving the flushInProgress = false; at the end.

[wzieba]

Ahh right, there was a last second change I added, and I messed up 😞 .

In my previous tests, I had flushInProgress = true; inside AsyncTask#onPreExecute which runs on the main thread so it was working fine. But now it's not, as you pointed out - change of threads takes time and the main thread does still have time to run new FlushQueue().execute(); twice.

I'll answer to other concerns in a separate comment 👍 .

[wzieba]

Thanks @ParaskP7 , you raise good concerns in the comment and I agree with them 👍. I can provide some more context:

What worries me is if there are other valid scenarios where flushEventQueue() needs to be triggered twice (or more), while the previous doInBackground(...) is still in-progress.

There's no valid scenario to call flushEventQueue() twice. The scenario I thought to cover with this fix was an invalid configuration of the SDK by users of it.

(...) the way we tested it, via the FLUSH EVENT QUEUE is something that can't/shouldn't be done manually in production apps using our SKD, and instead, only depend on the timer to only do that automatically, based on the flushInterval parameter, after a specific period of time.

Users are asked to call flushEventQueue in onDestroy of the Activity (details). So, with current interface, we have to offer flushEventQueue. Although I have concerns with this approach - making http request in the onDestroy will usually (if not always) fail, as it's already too late for this in app's lifecycle. But this is maybe a separate topic to discuss in P2.

Me keep writing and thinking about it, I wonder whether it would make more sense to somehow limit the flushEventQueue() to be an internal or private method, only accessed by the timer (see FlushManager.runningTask), and also, maybe even limit the flushInterval to a minimum allowed.

Yes, it'd be the best strategy IMO as well. I'll prepare a P2 proposal and then we can discuss it with the rest of the team 👍

---

Thanks for sharing your thoughts @ParaskP7 , it made me rethink the approach proposed here! I'll put this PR on draft for now.

[ParaskP7]

Thanks @ParaskP7 , you raise good concerns in the comment and I agree with them 👍. I can provide some more context:

Thank YOU @wzieba ! 🥇

There's no valid scenario to call flushEventQueue() twice. The scenario I thought to cover with this fix was an invalid configuration of the SDK by users of it.

Got it, thanks! 👍

Users are asked to call flushEventQueue in onDestroy of the Activity (details). So, with current interface, we have to offer flushEventQueue.

True... 🤷

Although I have concerns with this approach - making http request in the onDestroy will usually (if not always) fail, as it's already too late for this in app's lifecycle. But this is maybe a separate topic to discuss in P2.

Right, totally a separate topic for discussion, I agree! 👍

Yes, it'd be the best strategy IMO as well. I'll prepare a P2 proposal and then we can discuss it with the rest of the team 👍

Awesome, thanks! 🙇

Thanks for sharing your thoughts @ParaskP7 , it made me rethink the approach proposed here! I'll put this PR on draft for now.

Ah, I didn't want to block your work, apologies, this wasn't my intention here. Please think about it a bit and feel free to ask for my review when you are ready with another solution. 🙏

[wzieba]

Obsolete by #77