Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Race condition in LockFreeExponentiallyDecayingReservoir #85

Open
98amp2 opened this issue Sep 10, 2020 · 5 comments
Open

Race condition in LockFreeExponentiallyDecayingReservoir #85

98amp2 opened this issue Sep 10, 2020 · 5 comments

Comments

@98amp2
Copy link

98amp2 commented Sep 10, 2020

While using the version 1.0.7 in a backend, we noticed this error crop up

java.util.NoSuchElementException: null
at java.util.concurrent.ConcurrentSkipListMap.firstKey (ConcurrentSkipListMap.java:1858)
at com.spotify.metrics.core.LockFreeExponentiallyDecayingReservoir$State.update (LockFreeExponentiallyDecayingReservoir.java:207)
at com.spotify.metrics.core.LockFreeExponentiallyDecayingReservoir$State.update (LockFreeExponentiallyDecayingReservoir.java:198)
at com.spotify.metrics.core.LockFreeExponentiallyDecayingReservoir$State.access$200 (LockFreeExponentiallyDecayingReservoir.java:160)
at com.spotify.metrics.core.LockFreeExponentiallyDecayingReservoir.update (LockFreeExponentiallyDecayingReservoir.java:100)
at com.spotify.metrics.core.LockFreeExponentiallyDecayingReservoir.update (LockFreeExponentiallyDecayingReservoir.java:88)
at com.spotify.metrics.core.ReservoirWithTtl.update (ReservoirWithTtl.java:123)
at com.codahale.metrics.Histogram.update (Histogram.java:41)

It looks like this can occur if the States ConcurrentSkipListMap is reset via a backfill in one thread at the same time an update on that state happens in another thread. Granted it's a small window but it looks to be possible with the current implementation.
@malish8632
Copy link
Contributor

hi @98amp2 thanks for reporting. We will looking into this issue.

@rydenius rydenius mentioned this issue Oct 12, 2020
Closed
@spkrka
Copy link
Member

spkrka commented Oct 12, 2020

Sorry for introducing this regression!

I think the best fix would be to switch to this implementation instead: dropwizard/metrics#1656
We can either wait for it to be merged and released or we can inline it directly and replace this broken implementation.

@carterkozak would you be ok with us inlining that in this repo until it's released in dropwizard/metrics?

@carterkozak
Copy link

@carterkozak would you be ok with us inlining that in this repo until it's released in dropwizard/metrics?

Of course :-)
I’ve done the same here: https://github.com/palantir/tritium/blob/develop/tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/LockFreeExponentiallyDecayingReservoir.java
You could add a dependency on the tritium library if you prefer, however it may pull in additional transitive dependencies.

spkrka added a commit that referenced this issue Oct 12, 2020
@spkrka
Update to LockFreeExponentiallyDecayingReservoir
a8bfa06 
Code taken from dropwizard/metrics#1656
with permission from the author.
See: #85

Replace this with the class from the dropwizard dependency
once it is has been released.
@spkrka spkrka mentioned this issue Oct 12, 2020
Merged
@spkrka
Copy link
Member

spkrka commented Oct 12, 2020

@malish8632 I've made this PR now to address it: #87

spkrka added a commit that referenced this issue Oct 12, 2020
@spkrka
Update to LockFreeExponentiallyDecayingReservoir
dc7f34c 
Code taken from dropwizard/metrics#1656
with permission from the author.
See: #85

Replace this with the class from the dropwizard dependency
once it is has been released.
spkrka added a commit that referenced this issue Oct 12, 2020
@spkrka
Update to LockFreeExponentiallyDecayingReservoir
a481bd8 
Code taken from dropwizard/metrics#1656
with permission from the author.
See: #85

Replace this with the class from the dropwizard dependency
once it is has been released.
spkrka added a commit that referenced this issue Oct 12, 2020
@spkrka
Update to LockFreeExponentiallyDecayingReservoir
978d25b 
Code taken from dropwizard/metrics#1656
with permission from the author.
See: #85

Replace this with the class from the dropwizard dependency
once it is has been released.
spkrka added a commit that referenced this issue Oct 12, 2020
@spkrka
Update to LockFreeExponentiallyDecayingReservoir (#87)
9bcac26 
Code taken from dropwizard/metrics#1656
with permission from the author.
See: #85

Replace this with the class from the dropwizard dependency
once it is has been released.
@mattnworb
Copy link
Member

I think this can be closed now since #87 was merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@mattnworb @malish8632 @carterkozak @98amp2 @spkrka