[SDK-499] Synchronize EmbeddedSessionManager to fix embedded session races - #1083
Conversation
The NPE fix on start closed the reported crash but left the rest of the class open to the same threading. Callers reach EmbeddedSessionManager from arbitrary threads (issue #1052 reports Dispatchers.Default), and impressions is a plain LinkedHashMap: startImpression writes to it while endSession iterates it and then reassigns the field, so concurrent use could also throw ConcurrentModificationException or lose entries. The session field had the same check-then-act shape as start — two threads could both pass isTracking() and each track a session. Guard every access to impressions, session, and the impression fields with one private lock. The per-impression synchronized block is now redundant and removed; @volatile on start stays as visibility defence, but the class lock is the invariant. trackEmbeddedSession runs after the lock is released, since it calls back into IterableApi. endSession keeps its existing behaviour of doing nothing — not even resetting the session — when there are no impressions. Adds a test racing 8 threads over startImpression, pauseImpression, startSession and endSession, which reproduces the reported NullPointerException without this change, plus two tests pinning the endSession-with-no-impressions behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…edded-session-thread-safety
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jferrao-itrbl
left a comment
jferrao-itrbl
left a comment
There was a problem hiding this comment.
Approved with very minor comments.
| var displayCount: Int = 0, | ||
| var duration: Float = 0.0f, | ||
| var start: Date? = null | ||
| @Volatile var start: Date? = null |
There was a problem hiding this comment.
The start var is marked with @Volatile so every thread always sees the latest value. That extra marking is not needed, because the class already uses one lock for all of this data. It can make a later reader think the marking @Volatile is what stops the crash, when the lock is what actually does.
nit: Remove the extra @Volatile marking on start, or add a short comment that the lock is what keeps this safe.
| public void endSessionWithoutImpressionsLeavesSessionRunning() { | ||
| sessionManager.startSession(); | ||
| sessionManager.endSession(); | ||
|
|
||
| assertTrue(sessionManager.isTracking()); | ||
| } |
There was a problem hiding this comment.
This test checks that ending a session with no viewed messages still leaves the session running. Though that is a known bug, kept on purpose in this change, the test never says that in the file so someone could think this is the intended product behaviour.
nit: Add a one-line comment on the test: this pins a current bug; flip the assertion when ticket SDK-701 is done.
| @@ -108,13 +127,14 @@ public class EmbeddedSessionManager { | |||
| } | |||
There was a problem hiding this comment.
Maybe slightly out-of-scope but these two private methods walk and update the in-memory list of viewed messages without taking the lock themselves. Today that is fine, because the only caller already holds the lock. A later change could call them without the lock and bring races back.
nit: Add a short comment on both helpers: caller must already hold the lock.
Drop the redundant @volatile on EmbeddedImpressionData.start. Every read and write of it happens under the session manager's lock, so the annotation only suggested the field was safe on its own. Rename the three private helpers that touch impressions without locking to a Locked suffix, so the requirement is visible at each call site instead of only at the declaration. Note in the test that leaving the session open when there are no impressions is existing behavior being pinned, not intended behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…edded-session-thread-safety # Conflicts: # CHANGELOG.md
4 participants
📝 Summary
Makes
EmbeddedSessionManagerinternally thread-safe, fixing a reportedNullPointerExceptionplus two further races in the same class.🎟️ Jira Ticket: SDK-499
📖 Description
EmbeddedSessionManagerhad no synchronization at all, and its impression API has no SDK-internal callers — it is driven entirely by integrator code, so it can be entered from any thread. Three races: the reported NPE inupdateDisplayCountAndDuration()(null-check then!!onstart, whichpauseImpression()nulls in between), concurrent modification of the impression map, and duplicate session tracking whenendSession()raced with itself.All access to
session,impressionsand the impression fields now goes through one private lock. A single class-wide lock instead of a concurrent map because the invariants span several fields —endSession()has to end all impressions, snapshot the list and reset both fields as one step.trackEmbeddedSession()is called after the lock is released, since it re-entersIterableApiand integrator code.endSession()still leaves the session running when there are no impressions. That is pre-existing behavior, changing it changes what gets reported, so it is tracked separately in SDK-701. A test pins the current behavior so that change is explicit when SDK-701 lands.No public API change and no behavior change on the single-threaded path.
This development was started by contributor @Shamyyoun
🧪 How to test?
./gradlew :iterableapi:testDebugUnitTest --tests "com.iterable.iterableapi.EmbeddedSessionManagerThreadSafetyTest"concurrentSessionAndImpressionUpdatesDoNotThrowraces 8 threads over the session and impression API. Reverting the two source files to master reproduces the reported crash:Full
iterableapisuite passed locally.🧾 Changelog
NullPointerExceptioninEmbeddedSessionManager.updateDisplayCountAndDuration()that could crash apps calling embedded session methods off the main thread.EmbeddedSessionManageris now internally synchronized, which also fixes concurrent modification of its impression map and duplicate session tracking whenendSession()raced with itself. Thanks to @Shamyyoun for the report and initial fix.📹 Loom recording if applicable
N/A
🐞 Github Issues solved
Addresses #1052, supersedes #1053. Deliberately not using a closing keyword yet so merging doesn't auto-close the issue before we reply to the reporter.
📚 Docs PR if applicable
N/A