[SDK-499] Synchronize EmbeddedSessionManager to fix embedded session races - #1083
Open
franco-zalamena-iterable wants to merge 6 commits into
Open
[SDK-499] Synchronize EmbeddedSessionManager to fix embedded session races#1083franco-zalamena-iterable wants to merge 6 commits into
franco-zalamena-iterable wants to merge 6 commits into
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 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