amplitude / Amplitude-Android

Native Android SDK for Amplitude
MIT License
164 stars 90 forks source link

fix: try to correctly handle session change for race condition #382

Closed justin-fiedler closed 1 year ago

justin-fiedler commented 1 year ago

Description

We are still getting long sessions for some customers in Flutter on Android after the other session fixes.

This change aims to handle the hypothetical case where an event is tracked before the runOnLogThread(new Runnable() executes.

That scenario would lead the event being "in foreground" which would extend the existing session.

Maybe it is also possible to move setting inForeground to the first line of the onEnterForeground Runnable? wdyt @falconandy?

void onEnterForeground(final long timestamp) {
-        inForeground = true;
        runOnLogThread(new Runnable() {
            @Override
            public void run() {
+             inForeground = true;

https://github.com/amplitude/Amplitude-Android/pull/362/files

justin-fiedler commented 1 year ago

Hi @justin-fiedler. Earlier I moved inForeground setter/getter from log thread body to main thread to avoid possible data races. Now new data race with isEnteringForeground is added, I believe. Currently I do not see how the new flag can help - could you please provide an example with details to illustrate incorrect/fixed behavior? Maybe it makes sense to add some internal session-related metrics to event payload (for example, when time gap between events is too large) to help debug session issues.

Thanks @falconandy I reviewed with @qingzhuozhen and we feel that this change shouldn't cause any new problems and is worth trying to see if it resolves the issue.

We are in process adding diagnostics to the SDKs to make it easier to locate issues in the near future.

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 2.39.8 :tada:

The release is available on:

Your semantic-release bot :package::rocket: