Skip to content

Commit 5625283

Browse files
romtsnromtsn
authored
fix(sessions): Move and flush unfinished previous session on init (#4624)
* fix(sessions): Move and flush unfinished previous session on init * Changelog * Address PR review
1 parent 7f16a2c commit 5625283

File tree

8 files changed

+387
-31
lines changed

8 files changed

+387
-31
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
- Ensure frame metrics listeners are registered/unregistered on the main thread ([#4582](https://github.com/getsentry/sentry-java/pull/4582))
2929
- Do not report cached events as lost ([#4575](https://github.com/getsentry/sentry-java/pull/4575))
3030
- Previously events were recorded as lost early despite being retried later through the cache
31+
- Move and flush unfinished previous session on init ([#4624](https://github.com/getsentry/sentry-java/pull/4624))
32+
- This removes the need for unnecessary blocking our background queue for 15 seconds in the case of a background app start
3133
- Switch to compileOnly dependency for compose-ui-material ([#4630](https://github.com/getsentry/sentry-java/pull/4630))
3234
- This fixes `StackOverflowError` when using OSS Licenses plugin
3335

‎sentry/api/sentry.api‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4346,13 +4346,15 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache {
43464346
public static final field SUFFIX_ENVELOPE_FILE Ljava/lang/String;
43474347
protected static final field UTF_8 Ljava/nio/charset/Charset;
43484348
protected final field cacheLock Lio/sentry/util/AutoClosableReentrantLock;
4349+
protected final field sessionLock Lio/sentry/util/AutoClosableReentrantLock;
43494350
public fun <init> (Lio/sentry/SentryOptions;Ljava/lang/String;I)V
43504351
public static fun create (Lio/sentry/SentryOptions;)Lio/sentry/cache/IEnvelopeCache;
43514352
public fun discard (Lio/sentry/SentryEnvelope;)V
43524353
public fun flushPreviousSession ()V
43534354
public static fun getCurrentSessionFile (Ljava/lang/String;)Ljava/io/File;
43544355
public static fun getPreviousSessionFile (Ljava/lang/String;)Ljava/io/File;
43554356
public fun iterator ()Ljava/util/Iterator;
4357+
public fun movePreviousSession (Ljava/io/File;Ljava/io/File;)V
43564358
public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
43574359
public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
43584360
public fun waitPreviousSessionFlush ()Z

‎sentry/src/main/java/io/sentry/MovePreviousSession.java‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
package io.sentry;
2+
3+
import static io.sentry.SentryLevel.DEBUG;
4+
import static io.sentry.SentryLevel.INFO;
5+
6+
import io.sentry.cache.EnvelopeCache;
7+
import io.sentry.cache.IEnvelopeCache;
8+
import java.io.File;
9+
import org.jetbrains.annotations.NotNull;
10+
11+
final class MovePreviousSession implements Runnable {
12+
13+
private final @NotNull SentryOptions options;
14+
15+
MovePreviousSession(final @NotNull SentryOptions options) {
16+
this.options = options;
17+
}
18+
19+
@Override
20+
public void run() {
21+
final String cacheDirPath = options.getCacheDirPath();
22+
if (cacheDirPath == null) {
23+
options.getLogger().log(INFO, "Cache dir is not set, not moving the previous session.");
24+
return;
25+
}
26+
27+
if (!options.isEnableAutoSessionTracking()) {
28+
options
29+
.getLogger()
30+
.log(DEBUG, "Session tracking is disabled, bailing from previous session mover.");
31+
return;
32+
}
33+
34+
final IEnvelopeCache cache = options.getEnvelopeDiskCache();
35+
if (cache instanceof EnvelopeCache) {
36+
final File currentSessionFile = EnvelopeCache.getCurrentSessionFile(cacheDirPath);
37+
final File previousSessionFile = EnvelopeCache.getPreviousSessionFile(cacheDirPath);
38+
39+
((EnvelopeCache) cache).movePreviousSession(currentSessionFile, previousSessionFile);
40+
41+
((EnvelopeCache) cache).flushPreviousSession();
42+
}
43+
}
44+
}

‎sentry/src/main/java/io/sentry/Sentry.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,8 @@ private static void init(final @NotNull SentryOptions options, final boolean glo
346346
options.setExecutorService(new SentryExecutorService(options));
347347
options.getExecutorService().prewarm();
348348
}
349+
350+
movePreviousSession(options);
349351
// when integrations are registered on Scopes ctor and async integrations are fired,
350352
// it might and actually happened that integrations called captureSomething
351353
// and Scopes was still NoOp.
@@ -497,6 +499,16 @@ private static void handleAppStartProfilingConfig(
497499
return options.getInternalTracesSampler().sample(appStartSamplingContext);
498500
}
499501

502+
@SuppressWarnings("FutureReturnValueIgnored")
503+
private static void movePreviousSession(final @NotNull SentryOptions options) {
504+
// enqueue a task to move previous unfinished session to its own file
505+
try {
506+
options.getExecutorService().submit(new MovePreviousSession(options));
507+
} catch (Throwable e) {
508+
options.getLogger().log(SentryLevel.DEBUG, "Failed to move previous session.", e);
509+
}
510+
}
511+
500512
@SuppressWarnings("FutureReturnValueIgnored")
501513
private static void finalizePreviousSession(
502514
final @NotNull SentryOptions options, final @NotNull IScopes scopes) {

‎sentry/src/main/java/io/sentry/cache/EnvelopeCache.java‎

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ public class EnvelopeCache extends CacheStrategy implements IEnvelopeCache {
7373

7474
private final @NotNull Map<SentryEnvelope, String> fileNameMap = new WeakHashMap<>();
7575
protected final @NotNull AutoClosableReentrantLock cacheLock = new AutoClosableReentrantLock();
76+
protected final @NotNull AutoClosableReentrantLock sessionLock = new AutoClosableReentrantLock();
7677

7778
public static @NotNull IEnvelopeCache create(final @NotNull SentryOptions options) {
7879
final String cacheDirPath = options.getCacheDirPath();
@@ -123,20 +124,7 @@ private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @Not
123124
}
124125

125126
if (HintUtils.hasType(hint, SessionStart.class)) {
126-
if (currentSessionFile.exists()) {
127-
options.getLogger().log(WARNING, "Current session is not ended, we'd need to end it.");
128-
129-
try (final Reader reader =
130-
new BufferedReader(
131-
new InputStreamReader(new FileInputStream(currentSessionFile), UTF_8))) {
132-
final Session session = serializer.getValue().deserialize(reader, Session.class);
133-
if (session != null) {
134-
writeSessionToDisk(previousSessionFile, session);
135-
}
136-
} catch (Throwable e) {
137-
options.getLogger().log(SentryLevel.ERROR, "Error processing session.", e);
138-
}
139-
}
127+
movePreviousSession(currentSessionFile, previousSessionFile);
140128
updateCurrentSession(currentSessionFile, envelope);
141129

142130
boolean crashedLastRun = false;
@@ -329,17 +317,12 @@ private boolean writeEnvelopeToDisk(
329317
}
330318

331319
private void writeSessionToDisk(final @NotNull File file, final @NotNull Session session) {
332-
if (file.exists()) {
320+
try (final OutputStream outputStream = new FileOutputStream(file);
321+
final Writer writer = new BufferedWriter(new OutputStreamWriter(outputStream, UTF_8))) {
333322
options
334323
.getLogger()
335324
.log(DEBUG, "Overwriting session to offline storage: %s", session.getSessionId());
336-
if (!file.delete()) {
337-
options.getLogger().log(SentryLevel.ERROR, "Failed to delete: %s", file.getAbsolutePath());
338-
}
339-
}
340325

341-
try (final OutputStream outputStream = new FileOutputStream(file);
342-
final Writer writer = new BufferedWriter(new OutputStreamWriter(outputStream, UTF_8))) {
343326
serializer.getValue().serialize(session, writer);
344327
} catch (Throwable e) {
345328
options
@@ -454,4 +437,33 @@ public boolean waitPreviousSessionFlush() {
454437
public void flushPreviousSession() {
455438
previousSessionLatch.countDown();
456439
}
440+
441+
public void movePreviousSession(
442+
final @NotNull File currentSessionFile, final @NotNull File previousSessionFile) {
443+
try (final @NotNull ISentryLifecycleToken ignored = sessionLock.acquire()) {
444+
if (previousSessionFile.exists()) {
445+
options.getLogger().log(DEBUG, "Previous session file already exists, deleting it.");
446+
if (!previousSessionFile.delete()) {
447+
options
448+
.getLogger()
449+
.log(WARNING, "Unable to delete previous session file: %s", previousSessionFile);
450+
}
451+
}
452+
453+
if (currentSessionFile.exists()) {
454+
options.getLogger().log(INFO, "Moving current session to previous session.");
455+
456+
try {
457+
final boolean renamed = currentSessionFile.renameTo(previousSessionFile);
458+
if (!renamed) {
459+
options.getLogger().log(WARNING, "Unable to move current session to previous session.");
460+
}
461+
} catch (Throwable e) {
462+
options
463+
.getLogger()
464+
.log(SentryLevel.ERROR, "Error moving current session to previous session.", e);
465+
}
466+
}
467+
}
468+
}
457469
}

‎sentry/src/test/java/io/sentry/MovePreviousSessionTest.kt‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
package io.sentry
2+
3+
import io.sentry.cache.EnvelopeCache
4+
import io.sentry.cache.IEnvelopeCache
5+
import io.sentry.transport.NoOpEnvelopeCache
6+
import java.nio.file.Files
7+
import java.nio.file.Path
8+
import kotlin.test.AfterTest
9+
import kotlin.test.BeforeTest
10+
import kotlin.test.Test
11+
import kotlin.test.assertFalse
12+
import kotlin.test.assertTrue
13+
import org.mockito.kotlin.any
14+
import org.mockito.kotlin.mock
15+
import org.mockito.kotlin.never
16+
import org.mockito.kotlin.verify
17+
18+
class MovePreviousSessionTest {
19+
20+
private class Fixture {
21+
val tempDir: Path = Files.createTempDirectory("sentry-move-session-test")
22+
val options =
23+
SentryOptions().apply {
24+
isDebug = true
25+
setLogger(SystemOutLogger())
26+
}
27+
val cache = mock<EnvelopeCache>()
28+
29+
fun getSUT(
30+
cacheDirPath: String? = tempDir.toAbsolutePath().toFile().absolutePath,
31+
isEnableSessionTracking: Boolean = true,
32+
envelopeCache: IEnvelopeCache? = null,
33+
): MovePreviousSession {
34+
options.cacheDirPath = cacheDirPath
35+
options.isEnableAutoSessionTracking = isEnableSessionTracking
36+
options.setEnvelopeDiskCache(envelopeCache ?: EnvelopeCache.create(options))
37+
return MovePreviousSession(options)
38+
}
39+
40+
fun cleanup() {
41+
tempDir.toFile().deleteRecursively()
42+
}
43+
}
44+
45+
private lateinit var fixture: Fixture
46+
47+
@BeforeTest
48+
fun setup() {
49+
fixture = Fixture()
50+
}
51+
52+
@AfterTest
53+
fun teardown() {
54+
fixture.cleanup()
55+
}
56+
57+
@Test
58+
fun `when cache dir is null, logs and returns early`() {
59+
val sut = fixture.getSUT(cacheDirPath = null, envelopeCache = fixture.cache)
60+
61+
sut.run()
62+
63+
verify(fixture.cache, never()).movePreviousSession(any(), any())
64+
verify(fixture.cache, never()).flushPreviousSession()
65+
}
66+
67+
@Test
68+
fun `when session tracking is disabled, logs and returns early`() {
69+
val sut = fixture.getSUT(isEnableSessionTracking = false, envelopeCache = fixture.cache)
70+
71+
sut.run()
72+
73+
verify(fixture.cache, never()).movePreviousSession(any(), any())
74+
verify(fixture.cache, never()).flushPreviousSession()
75+
}
76+
77+
@Test
78+
fun `when envelope cache is not EnvelopeCache instance, does nothing`() {
79+
val sut = fixture.getSUT(envelopeCache = NoOpEnvelopeCache.getInstance())
80+
81+
sut.run()
82+
83+
verify(fixture.cache, never()).movePreviousSession(any(), any())
84+
verify(fixture.cache, never()).flushPreviousSession()
85+
}
86+
87+
@Test
88+
fun `integration test with real EnvelopeCache`() {
89+
val sut = fixture.getSUT()
90+
91+
// Create a current session file
92+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
93+
val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
94+
95+
currentSessionFile.createNewFile()
96+
currentSessionFile.writeText("session content")
97+
98+
assertTrue(currentSessionFile.exists())
99+
assertFalse(previousSessionFile.exists())
100+
101+
sut.run()
102+
103+
// Wait for flush to complete
104+
(fixture.options.envelopeDiskCache as EnvelopeCache).waitPreviousSessionFlush()
105+
106+
// Current session file should have been moved to previous
107+
assertFalse(currentSessionFile.exists())
108+
assertTrue(previousSessionFile.exists())
109+
assert(previousSessionFile.readText() == "session content")
110+
111+
fixture.cleanup()
112+
}
113+
114+
@Test
115+
fun `integration test when current session file does not exist`() {
116+
val sut = fixture.getSUT()
117+
118+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
119+
val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
120+
121+
assertFalse(currentSessionFile.exists())
122+
assertFalse(previousSessionFile.exists())
123+
124+
sut.run()
125+
126+
(fixture.options.envelopeDiskCache as EnvelopeCache).waitPreviousSessionFlush()
127+
128+
assertFalse(currentSessionFile.exists())
129+
assertFalse(previousSessionFile.exists())
130+
131+
fixture.cleanup()
132+
}
133+
134+
@Test
135+
fun `integration test when previous session file already exists`() {
136+
val sut = fixture.getSUT()
137+
138+
val currentSessionFile = EnvelopeCache.getCurrentSessionFile(fixture.options.cacheDirPath!!)
139+
val previousSessionFile = EnvelopeCache.getPreviousSessionFile(fixture.options.cacheDirPath!!)
140+
141+
currentSessionFile.createNewFile()
142+
currentSessionFile.writeText("current session")
143+
previousSessionFile.createNewFile()
144+
previousSessionFile.writeText("previous session")
145+
146+
assertTrue(currentSessionFile.exists())
147+
assertTrue(previousSessionFile.exists())
148+
149+
sut.run()
150+
151+
(fixture.options.envelopeDiskCache as EnvelopeCache).waitPreviousSessionFlush()
152+
153+
// Current session file should have been moved to previous
154+
assertFalse(currentSessionFile.exists())
155+
assertTrue(previousSessionFile.exists())
156+
assert(previousSessionFile.readText() == "current session")
157+
158+
fixture.cleanup()
159+
}
160+
}

0 commit comments

Comments
 (0)