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.

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

prevent devious scheduling from reporting negative size to kamon #624

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

schlawg
Copy link
Contributor

@schlawg schlawg commented Nov 28, 2024

ConcurrentLinkedQueue.add is not atomic with respect to AtomicInteger.getAndIncrement. I think we should increment the size variable before we call add to address this race condition:

Thread A is executing the flush() loop.
Thread B calls flushQ.add, the channel is added, but the thread is interrupted before incrementing size.
Thread A empties the queue and now flushQ.size is actually -1.
Thread A takes a 1ms nap while thread B remains suspended.
Thread A wakes up and enters flush() again, assigning flushQ.estimateSize() (which is still -1) to qSize
Thread B finally resumes and corrects the size variable, but the spurious negative value has already been captured and will be reported to kamon.

A negative could spike the y-axis scale if grafana or another process in the reporting chain is configured to expect unsigned values.

@schlawg
Copy link
Contributor Author

schlawg commented Nov 28, 2024

The PR was inspired by this graph

@ornicar ornicar merged commit 9dc34d5 into lichess-org:master Nov 28, 2024
1 check passed
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.

2 participants