Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix metrics reporting in applications using
fork
s #205Fix metrics reporting in applications using
fork
s #205Changes from 4 commits
e1e00b5
9756575
38758f4
a3375b2
dc24b67
073f685
5aedbb8
362acc7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Hmm I still see a potential issue here, similar to the ones in
#flush
and#rendez_vous
: with some poor timing ofstop
, by the time we get to themessage_queue << message
line,message_queue
may benil
.(And its cousin issue,
@sender_thread
beingnil
)I think part of the issue is that we have the background thread setting these two things to
nil
without ever synchronizing with any other threads, which can get surprised by this at many points in their execution. While we could expand the synchronization even more, that seems to me to be a bit heavy-handed, especially since we have a thread-safe construct (Queue
) that we're building around in this class.Here's my suggestion:
Queue
in#initialize
nil
or close it (but we may#clear
it when restarting the background thread or after a stop). This enables us to always know that we can safely use it and call methods on it.@sender_thread
-> starting it, changing it (when it dies), or setting it tonil
(when it finishes due to#stop
). Reading@sender_thread
for checks is OK to do without locks.This is just a suggestion, so feel free to ignore and do something else. But I think this class as it is, is hiding a lot of complexity introduced by the shared mutable state, truly the more I look the more I see potential issues.
Let me know if you'd like to pair on this; perhaps that way we can get this across the finish line without so much async back-and-forth and rework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is why at first I've used the mutex to synchronize the close as well, it would avoid timing with the
#stop
call since the#stop
would be able to run only if it has the lock. I'll see what change represents your suggestion, and I agree that this class ships a lot of complexity now...