fix: try to correctly handle session change for race condition#382
Conversation
falconandy
left a comment
There was a problem hiding this comment.
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. |
Fix shouldn't cause new problems. Deploying to see if it resolves the issue for the customer.
## [2.39.8](v2.39.7...v2.39.8) (2023-07-21) ### Bug Fixes * try to correctly handle session change for race condition ([#382](#382)) ([b0f4fea](b0f4fea))
|
🎉 This PR is included in version 2.39.8 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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
inForegroundto 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