Skip to content

Set the userId on the analytics executor thread.#573

Merged
f2prateek merged 1 commit intomasterfrom
fix-user-id-thread
Feb 5, 2018
Merged

Set the userId on the analytics executor thread.#573
f2prateek merged 1 commit intomasterfrom
fix-user-id-thread

Conversation

@f2prateek
Copy link
Copy Markdown
Contributor

Previously we were setting the userId on the thread that identify is called (which may be any thread), but reading the userId on a background thread.

Because of this, the userId on events called before identify may have the userId from "newer" events.

analytics.identify("foo");
analytics.track("event"); -> may have userId foo
analytics.identify("bar");

To fix this, we now also set the userId on the background thread.

Originally referenced in #522. Note that my original solution proposed we read the userId on the main thread to fix the issue, but the fix implemented here is to read the userId on the background thread instead.

Ref: LIB-120

Closes #522

Previously we were setting the userId on the thread that `identify` is called (which may be any thread), but reading the userId on a background thread.

Because of this, the userId on events called before identify may have the userId from "newer" events.

```
analytics.identify("foo");
analytics.track("event"); -> may have userId foo
analytics.identify("bar");
```

To fix this, we now also set the userId on the background thread.

Originally referenced in #522. Note that my original solution proposed we read the userId on the main thread to fix the issue, but the fix implemented here is to read the userId on the background thread instead.

Ref: LIB-120

Closes #522
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #573 into master will not change coverage.
The diff coverage is 85.71%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #573   +/-   ##
=======================================
  Coverage   82.94%   82.94%           
=======================================
  Files          30       30           
  Lines        2451     2451           
  Branches      281      281           
=======================================
  Hits         2033     2033           
  Misses        334      334           
  Partials       84       84
Impacted Files Coverage Δ
...src/main/java/com/segment/analytics/Analytics.java 75.34% <85.71%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8570331...c7a4f1d. Read the comment docs.

@f2prateek f2prateek merged commit fb11f42 into master Feb 5, 2018
@f2prateek f2prateek deleted the fix-user-id-thread branch February 5, 2018 19:48
@flagbug
Copy link
Copy Markdown

flagbug commented Jan 24, 2019

@f2prateek Is there a plan to ever release this fix in a stable version? I think we're hitting this issue, but since we've pinned the Segment library to stable versions, it looks like this has never been picked up in our app...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UserID should be recorded at time of event.

3 participants