Update SessionTracker to use BackgroundTaskService

CHANGELOG.md

@@ -14,6 +14,7 @@
   [#1165](https://github.com/bugsnag/bugsnag-android/pull/1165)
   [#1164](https://github.com/bugsnag/bugsnag-android/pull/1164)
   [#1182](https://github.com/bugsnag/bugsnag-android/pull/1182)
+  [#1184](https://github.com/bugsnag/bugsnag-android/pull/1184)
 
 ## 5.7.0 (2021-02-18)
 

bugsnag-android-core/src/androidTest/java/com/bugsnag/android/SessionTrackerConfinementTest.kt

@@ -0,0 +1,58 @@
+package com.bugsnag.android
+
+import androidx.test.core.app.ApplicationProvider
+import org.junit.Assert.assertEquals
+import org.junit.Test
+import java.util.Date
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.TimeUnit
+
+private const val SESSION_CONFINEMENT_ATTEMPTS = 20
+
+/**
+ * Confirms that delivery of sessions is confined to a single thread, resulting in no
+ * duplicate requests.
+ */
+internal class SessionTrackerConfinementTest {
+
+    @Test
+    fun trackingSessionsIsThreadConfined() {
+        // setup delivery for interception
+        val retainingDelivery = RetainingDelivery()
+        val config = BugsnagTestUtils.generateConfiguration().apply {
+            autoTrackSessions = false
+            delivery = retainingDelivery
+        }
+        val client = Client(ApplicationProvider.getApplicationContext(), config)
+
+        // send 20 sessions
+        repeat(SESSION_CONFINEMENT_ATTEMPTS) { count ->
+            client.sessionTracker.startNewSession(Date(0), User("$count"), false)
+        }
+        retainingDelivery.latch.await(1, TimeUnit.SECONDS)
+
+        // confirm that no dupe requests are sent and that the request order is deterministic
+        assertEquals(SESSION_CONFINEMENT_ATTEMPTS, retainingDelivery.payloads.size)
+
+        retainingDelivery.payloads.forEachIndexed { index, session ->
+            assertEquals("$index", session.getUser().id)
+        }
+    }
+
+    /**
+     * Retains all the sent session payloads
+     */
+    private class RetainingDelivery : Delivery {
+        val payloads = mutableListOf<Session>()
+        val latch = CountDownLatch(SESSION_CONFINEMENT_ATTEMPTS)
+
+        override fun deliver(payload: Session, deliveryParams: DeliveryParams): DeliveryStatus {
+            payloads.add(payload)
+            latch.countDown()
+            return DeliveryStatus.DELIVERED
+        }
+
+        override fun deliver(payload: EventPayload, deliveryParams: DeliveryParams) =
+            DeliveryStatus.DELIVERED
+    }
+}

bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java

@@ -85,6 +85,7 @@ public class Client implements MetadataAware, CallbackAware, UserAware {
     LastRunInfo lastRunInfo;
     final LastRunInfoStore lastRunInfoStore;
     final LaunchCrashTracker launchCrashTracker;
+    final BackgroundTaskService bgTaskService = new BackgroundTaskService();
 
     /**
      * Initialize a Bugsnag client
@@ -147,7 +148,7 @@ public Unit invoke(Boolean hasConnection, String networkState) {
 
         sessionStore = new SessionStore(immutableConfig, logger, null);
         sessionTracker = new SessionTracker(immutableConfig, callbackState, this,
-                sessionStore, logger);
+                sessionStore, logger, bgTaskService);
         metadataState = copyMetadataState(configuration);
 
         ActivityManager am =
@@ -963,8 +964,10 @@ void addRuntimeVersionInfo(@NonNull String key, @NonNull String value) {
         deviceDataCollector.addRuntimeVersionInfo(key, value);
     }
 
+    @VisibleForTesting
     void close() {
         connectivity.unregisterForNetworkChanges();
+        bgTaskService.shutdown();
     }
 
     Logger getLogger() {

bugsnag-android-core/src/main/java/com/bugsnag/android/SessionTracker.java

@@ -12,7 +12,6 @@
 import java.util.UUID;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.RejectedExecutionException;
-import java.util.concurrent.Semaphore;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
 
@@ -35,23 +34,34 @@ class SessionTracker extends BaseObservable {
     // The first Activity in this 'session' was started at this time.
     private final AtomicLong lastEnteredForegroundMs = new AtomicLong(0);
     private final AtomicReference<Session> currentSession = new AtomicReference<>();
-    private final Semaphore flushingRequest = new Semaphore(1);
     private final ForegroundDetector foregroundDetector;
+    final BackgroundTaskService backgroundTaskService;
     final Logger logger;
 
-    SessionTracker(ImmutableConfig configuration, CallbackState callbackState,
-                   Client client, SessionStore sessionStore, Logger logger) {
-        this(configuration, callbackState, client, DEFAULT_TIMEOUT_MS, sessionStore, logger);
+    SessionTracker(ImmutableConfig configuration,
+                   CallbackState callbackState,
+                   Client client,
+                   SessionStore sessionStore,
+                   Logger logger,
+                   BackgroundTaskService backgroundTaskService) {
+        this(configuration, callbackState, client, DEFAULT_TIMEOUT_MS,
+                sessionStore, logger, backgroundTaskService);
     }
 
-    SessionTracker(ImmutableConfig configuration, CallbackState callbackState,
-                   Client client, long timeoutMs, SessionStore sessionStore, Logger logger) {
+    SessionTracker(ImmutableConfig configuration,
+                   CallbackState callbackState,
+                   Client client,
+                   long timeoutMs,
+                   SessionStore sessionStore,
+                   Logger logger,
+                   BackgroundTaskService backgroundTaskService) {
         this.configuration = configuration;
         this.callbackState = callbackState;
         this.client = client;
         this.timeoutMs = timeoutMs;
         this.sessionStore = sessionStore;
         this.foregroundDetector = new ForegroundDetector(client.getAppContext());
+        this.backgroundTaskService = backgroundTaskService;
         this.logger = logger;
         notifyNdkInForeground();
     }
@@ -159,41 +169,8 @@ private void trackSessionIfNeeded(final Session session) {
                 && session.isTracked().compareAndSet(false, true)) {
             notifySessionStartObserver(session);
 
-            try {
-                Async.run(new Runnable() {
-                    @Override
-                    public void run() {
-                        //FUTURE:SM It would be good to optimise this
-                        flushStoredSessions();
-
-                        try {
-                            logger.d("SessionTracker#trackSessionIfNeeded() "
-                                + "- attempting initial delivery");
-                            DeliveryStatus deliveryStatus = deliverSessionPayload(session);
-
-                            switch (deliveryStatus) {
-                                case UNDELIVERED:
-                                    logger.w("Storing session payload for future delivery");
-                                    sessionStore.write(session);
-                                    break;
-                                case FAILURE:
-                                    logger.w("Dropping invalid session tracking payload");
-                                    break;
-                                case DELIVERED:
-                                    logger.d("Sent 1 new session to Bugsnag");
-                                    break;
-                                default:
-                                    break;
-                            }
-                        } catch (Exception exception) {
-                            logger.w("Session tracking payload failed", exception);
-                        }
-                    }
-                });
-            } catch (RejectedExecutionException exception) {
-                // This is on the current thread but there isn't much else we can do
-                sessionStore.write(session);
-            }
+            flushAsync();
+            flushInMemorySession(session);
         }
     }
 
@@ -240,7 +217,7 @@ Session incrementHandledAndCopy() {
      */
     void flushAsync() {
         try {
-            Async.run(new Runnable() {
+            backgroundTaskService.submitTask(TaskType.SESSION_REQUEST, new Runnable() {
                 @Override
                 public void run() {
                     flushStoredSessions();
@@ -255,16 +232,10 @@ public void run() {
      * Attempts to flush session payloads stored on disk
      */
     void flushStoredSessions() {
-        if (flushingRequest.tryAcquire(1)) {
-            try {
-                List<File> storedFiles = sessionStore.findStoredFiles();
+        List<File> storedFiles = sessionStore.findStoredFiles();
 
-                for (File storedFile : storedFiles) {
-                    flushStoredSession(storedFile);
-                }
-            } finally {
-                flushingRequest.release(1);
-            }
+        for (File storedFile : storedFiles) {
+            flushStoredSession(storedFile);
         }
     }
 
@@ -298,6 +269,44 @@ void flushStoredSession(File storedFile) {
         }
     }
 
+    private void flushInMemorySession(final Session session) {
+        try {
+            backgroundTaskService.submitTask(TaskType.SESSION_REQUEST, new Runnable() {
+                @Override
+                public void run() {
+                    deliverInMemorySession(session);
+                }
+            });
+        } catch (RejectedExecutionException exception) {
+            // This is on the current thread but there isn't much else we can do
+            sessionStore.write(session);
+        }
+    }
+
+    void deliverInMemorySession(Session session) {
+        try {
+            logger.d("SessionTracker#trackSessionIfNeeded() - attempting initial delivery");
+            DeliveryStatus deliveryStatus = deliverSessionPayload(session);
+
+            switch (deliveryStatus) {
+                case UNDELIVERED:
+                    logger.w("Storing session payload for future delivery");
+                    sessionStore.write(session);
+                    break;
+                case FAILURE:
+                    logger.w("Dropping invalid session tracking payload");
+                    break;
+                case DELIVERED:
+                    logger.d("Sent 1 new session to Bugsnag");
+                    break;
+                default:
+                    break;
+            }
+        } catch (Exception exception) {
+            logger.w("Session tracking payload failed", exception);
+        }
+    }
+
     DeliveryStatus deliverSessionPayload(Session payload) {
         DeliveryParams params = configuration.getSessionApiDeliveryParams();
         Delivery delivery = configuration.getDelivery();

bugsnag-android-core/src/test/java/com/bugsnag/android/SessionTrackerPauseResumeTest.kt

@@ -60,7 +60,8 @@ internal class SessionTrackerPauseResumeTest {
             configuration.impl.callbackState,
             client,
             sessionStore,
-            NoopLogger
+            NoopLogger,
+            BackgroundTaskService()
         )
     }
 

bugsnag-android-core/src/test/java/com/bugsnag/android/SessionTrackerTest.java

@@ -28,6 +28,7 @@ public class SessionTrackerTest {
     private User user;
     private Configuration configuration;
     private ImmutableConfig immutableConfig;
+    private final BackgroundTaskService bgTaskService = new BackgroundTaskService();
 
     @Mock
     Client client;
@@ -67,7 +68,8 @@ public void setUp() {
         configuration.setDelivery(BugsnagTestUtils.generateDelivery());
         immutableConfig = BugsnagTestUtils.generateImmutableConfig();
         sessionTracker = new SessionTracker(immutableConfig,
-                configuration.impl.callbackState, client, sessionStore, NoopLogger.INSTANCE);
+                configuration.impl.callbackState, client, sessionStore, NoopLogger.INSTANCE,
+                bgTaskService);
         configuration.setAutoTrackSessions(true);
         user = new User(null, null, null);
     }
@@ -156,7 +158,7 @@ public void testBasicInForeground() {
     public void testZeroSessionTimeout() {
         CallbackState callbackState = configuration.impl.callbackState;
         sessionTracker = new SessionTracker(immutableConfig, callbackState, client,
-            0, sessionStore, NoopLogger.INSTANCE);
+            0, sessionStore, NoopLogger.INSTANCE, bgTaskService);
 
         long now = System.currentTimeMillis();
         sessionTracker.updateForegroundTracker(ACTIVITY_NAME, true, now);
@@ -172,7 +174,7 @@ public void testZeroSessionTimeout() {
     public void testSessionTimeout() {
         CallbackState callbackState = configuration.impl.callbackState;
         sessionTracker = new SessionTracker(immutableConfig, callbackState, client,
-            100, sessionStore, NoopLogger.INSTANCE);
+            100, sessionStore, NoopLogger.INSTANCE, bgTaskService);
 
         long now = System.currentTimeMillis();
         sessionTracker.updateForegroundTracker(ACTIVITY_NAME, true, now);