Fix race conditions in channel #303

guperrot · 2017-01-14T01:29:42Z

No description provided.

msftclas · 2017-01-14T01:29:45Z

Hi @guperrot, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Guillaume Perrot). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

codecov-io · 2017-01-14T01:41:52Z

Current coverage is 100% (diff: 100%)

Merging #303 into develop will not change coverage

@@           develop   #303   diff @@
=====================================
  Files           60     60          
  Lines         2711   2725    +14   
  Methods          0      0          
  Messages         0      0          
  Branches       494    499     +5   
=====================================
+ Hits          2711   2725    +14   
  Misses           0      0          
  Partials         0      0

Powered by Codecov. Last update f504ef8...c0c5a04

jaeklim · 2017-01-17T17:31:28Z

sdk/mobile-center/src/main/java/com/microsoft/azure/mobile/channel/DefaultChannel.java

+            MobileCenterLog.debug(LOG_TAG, "enqueue(" + groupState.mName + ") pendingLogCount=" + groupState.mPendingLogCount);
+
+            /* Increment counters and schedule ingestion if we are not disabled. */
+            if (!mEnabled) {


Can you remove ! and swap if and else body?

jaeklim · 2017-01-17T17:34:19Z

sdk/mobile-center/src/main/java/com/microsoft/azure/mobile/channel/DefaultChannel.java

@@ -339,93 +369,103 @@ private synchronized void triggerIngestion(final @NonNull String groupName) {

        /* Get a batch from Persistence. */
        final List<Log> batch = new ArrayList<>(groupState.mMaxLogsPerBatch);
+        final int currentState = mStateChangeCounter;


Can you rename the local variable? Getting confused about State, better to have Counter

I will rename both to make it more explicit.

jaeklim · 2017-01-17T17:42:58Z

sdk/mobile-center/src/main/java/com/microsoft/azure/mobile/channel/DefaultChannel.java

            }
        });
    }

+    private synchronized void deleteLogsOnSuspended(GroupState groupState, int currentState, List<Log> logs) {


This method is a recursion. Once the Channel starts deleting logs on suspend, it should keep deleting even though the state gets changed in the middle. This is more like a PM question but we need to decide whether Channel keeps calling failure callbacks upon state change or not.

No it should not proceed, the disabling was cancelled, IO should stop at first chance we get.

We have to make sure that the logs should be gone at the time it is disabled.
It will send logs after re-enable that were created before it was disabled. I expect all the logs will be discarded after it is disabled. When it is enabled again, it should be in a clean states without any pending logs.

Settled offline with team, it's better to accidentally send logs that were disabled than accidentally call back old logs with a failure callback after it's already re-enabled. Either way it's a weird corner case but we had to choose between the two.

jaeklim · 2017-01-17T17:44:02Z

sdk/mobile-center/src/main/java/com/microsoft/azure/mobile/channel/DefaultChannel.java

+            if (logs.size() >= CLEAR_BATCH_SIZE && groupState.mListener != null) {
+                deleteLogsOnSuspended(groupState);
+            } else {
+                mPersistence.deleteLogs(groupState.mName);


If state gets changed, it will stop deleting logs for previous calls. Seems not right. Just checkStateDidNotChange out of the method is enough I think.

When state change, we should stop what we are doing at the first chance we get.

That is right but I think it is not in this case.

jaeklim · 2017-01-17T17:53:39Z

sdk/mobile-center/src/main/java/com/microsoft/azure/mobile/channel/DefaultChannel.java

+     * State checker. If this counter changes during a call to persistence, we have to ignore the result in the callback.
+     * Cancelling a database call would be unreliable, and if it's too fast you could still have the callback being called.
+     */
+    private int mStateChangeCounter;


This variable can only be changed in synchronized method as of now so it is not a problem at all. However I recommend to use AtomicReference just in case to prevent any mistakes from future implementations.
Just my opinion but what do you think?

They say in Javadoc that its not a general replacement for int, and it has overhead and we still need synchronized for general thread safety (even before this patch the code was meant to be thread safe).

Bitrise does not seem to support the previous version anymore.

jaeklim · 2017-01-17T21:05:46Z

...bile-center/src/test/java/com/microsoft/azure/mobile/channel/AbstractDefaultChannelTest.java

+@PrepareForTest({DefaultChannel.class, IdHelper.class, DeviceInfoHelper.class, DatabasePersistenceAsync.class, MobileCenterLog.class})
+public class AbstractDefaultChannelTest {
+
+    static final String TEST_GROUP = "group_test";


protected for all defaults?

Code analysis would complain since they are in same package.

It won't complain (I tried). We can change that later if we have any sub-classes in other packages.

Fix race conditions in channel

2730fdb

guperrot added the do not merge label Jan 14, 2017

msftclas added the cla-not-required label Jan 14, 2017

guperrot removed the do not merge label Jan 14, 2017

guperrot requested a review from jaeklim January 14, 2017 01:33

guperrot assigned jaeklim Jan 14, 2017

jaeklim reviewed Jan 17, 2017

View reviewed changes

Refactoring in channel

9a4b7a0

guperrot force-pushed the fix_channel_race_conditions branch from c0a1a0c to 9a4b7a0 Compare January 17, 2017 19:26

guperrot added 2 commits January 17, 2017 11:31

Update build tools

a74274d

Bitrise does not seem to support the previous version anymore.

Refactor a unit test to avoid a test race condition

c0c5a04

jaeklim reviewed Jan 17, 2017

View reviewed changes

jaeklim approved these changes Jan 17, 2017

View reviewed changes

guperrot merged commit 2b65219 into develop Jan 17, 2017

guperrot deleted the fix_channel_race_conditions branch January 17, 2017 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race conditions in channel #303

Fix race conditions in channel #303

guperrot commented Jan 14, 2017

msftclas commented Jan 14, 2017

codecov-io commented Jan 14, 2017 •

edited

Loading

jaeklim Jan 17, 2017

guperrot Jan 17, 2017

jaeklim Jan 17, 2017

guperrot Jan 17, 2017

jaeklim Jan 17, 2017

guperrot Jan 17, 2017 •

edited

Loading

jaeklim Jan 17, 2017

guperrot Jan 17, 2017

jaeklim Jan 17, 2017

guperrot Jan 17, 2017 •

edited

Loading

jaeklim Jan 17, 2017

jaeklim Jan 17, 2017

guperrot Jan 17, 2017

jaeklim Jan 17, 2017 •

edited

Loading

guperrot Jan 17, 2017

jaeklim Jan 17, 2017

Fix race conditions in channel #303

Fix race conditions in channel #303

Conversation

guperrot commented Jan 14, 2017

msftclas commented Jan 14, 2017

codecov-io commented Jan 14, 2017 • edited Loading

Current coverage is 100% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guperrot Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guperrot Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeklim Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 14, 2017 •

edited

Loading

guperrot Jan 17, 2017 •

edited

Loading

guperrot Jan 17, 2017 •

edited

Loading

jaeklim Jan 17, 2017 •

edited

Loading