Do not immediataly retry on network errors - #15
Conversation
There was a problem hiding this comment.
Could it be the case that this value is overwritten before it's being compared?
For example:
- We set the error timestamp to currentTimeMs (262)
- The bufferCopyThread begins inserting the mustKeepEvents to the DB (88)
- The network thread processes another event with success, sets lastNetworkErrorTimestamp to 0 (278)
- sendToWireThread begins processing events in the database (121)
- lastNetworkErrorTimestamp is 0 -> shouldWait is false (125)
- There is no pending flush so the events are not enqueued in the network thread (127)
Would be bizarre to have the network thread process another message before the sendToWire thread starts running but it looks like it's possible.
I think this will prevent that with another side-effect that the retry will happen two minutes from the first error instead of the last error.
if ((mPendingFlush || (!shouldWait && EventTable.getEventsCount(mContext) > DEFAULT_EVENTS_QUEUE_THREESHOLD))
&& NetworkUtils.isNetworkAvailable(mContext)) {
lastNetworkErrorTimestamp = 0L;
...
}
if (lastNetworkErrorTimestamp == 0L && isErrorResponse) {
lastNetworkErrorTimestamp = System.currentTimeMillis();
...
}
There was a problem hiding this comment.
I'm happy that you've found a flow path that could lead to a weird behavior.
Even if I'm not sure I've 100% understood the flow above :)
It's unlikely that the network thread process other messages immediately after an error, because it synchronizes over mNetworkQueue that should be empty at that point.
mNetworkQueue usually contains 1 element only, with all the events that should be sent over the wire with a single REST call.
If I got it right, the problem you've mentioned could happen if the app sends 10+ events to the library, immediately after that sendToWireThread has written to the queue. In that case the networkThread will find another event in the queue and start to precess it without waiting the timeout.
I think it's a rare case, and never get into it during the tests.
There was a problem hiding this comment.
If I got it right, the problem you've mentioned could happen if the app sends 10+ events to the library
That's exactly it.
mNetworkQueue usually contains 1 element only, with all the events that should be sent over the wire with a single REST call.
Because of this I think we're safe. Even if it was the case that many elements are in the queue this code-path could only hit if the network latency was somehow faster than a simple thread-wake. So...it would likely never occur. Looks like it's good for a merge 👍
|
|
…try-network-error Do not immediataly retry on network errors
|
I'll release an update today and get the WPAndroid PR merged in as well. |
Fix #14 by adding a fixed time of 2 minutes before retrying the network request. Otherwise there is a very high possibility to fall in a loop where the network connection is retried every second.
Also bumps version number to 1.0.4