Skip to content

Commit

Permalink
Merge pull request #1184 from bugsnag/session-tracker-bg-changes
Browse files Browse the repository at this point in the history 
Update SessionTracker to use BackgroundTaskService
  • Loading branch information
@fractalwrench
fractalwrench authored Mar 11, 2021
2 parents ed0f42e + 712c87a commit b697813
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 57 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
58 changes: 58 additions & 0 deletions ...ag-android-core/src/androidTest/java/com/bugsnag/android/SessionTrackerConfinementTest.kt
View file
Original file line number Diff line number Diff line change
@@ -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
}
}
5 changes: 4 additions & 1 deletion bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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() {
Expand Down
113 changes: 61 additions & 52 deletions bugsnag-android-core/src/main/java/com/bugsnag/android/SessionTracker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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();
}
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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();
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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();
Expand Down
3 changes: 2 additions & 1 deletion bugsnag-android-core/src/test/java/com/bugsnag/android/SessionTrackerPauseResumeTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ internal class SessionTrackerPauseResumeTest {
configuration.impl.callbackState,
client,
sessionStore,
NoopLogger
NoopLogger,
BackgroundTaskService()
)
}

Expand Down
8 changes: 5 additions & 3 deletions bugsnag-android-core/src/test/java/com/bugsnag/android/SessionTrackerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit b697813

Please sign in to comment.