Fix metrics reporting in applications using forks
#205
Conversation
|
(And thanks for the patience with my slow review turnaround) |
|
Hey @driv3r, thanks for the feedback and the comment. I definitely agree that a lot is happening here, but the idea would be to not have people going through a lot of documentation (or missing it) and opening issues because their framework is using forks. Plus, |
lib/datadog/statsd/sender.rb
Outdated
| def add(message) | ||
| raise ArgumentError, 'Start sender first' unless message_queue | ||
|
|
||
| # if the thread does not exist, we assume we are running in a forked process, | ||
| # empty the message queue and message buffers (these messages belong to | ||
| # the parent process) and spawn a new companion thread. | ||
| if !sender_thread.alive? | ||
| @mx.synchronize { | ||
| # a call from another thread has already re-created | ||
| # the companion thread before this one acquired the lock | ||
| break if sender_thread.alive? | ||
| @logger.debug { "Statsd: companion thread is dead, re-creating one" } if @logger | ||
|
|
||
| message_queue.close if CLOSEABLE_QUEUES | ||
| @message_queue = nil | ||
| message_buffer.reset | ||
| message_buffer.reset_telemetry | ||
| start | ||
| } | ||
| end | ||
|
|
||
| message_queue << message | ||
| end |
There was a problem hiding this comment.
🤔 Hmm I still see a potential issue here, similar to the ones in #flush and #rendez_vous: with some poor timing of stop, by the time we get to the message_queue << message line, message_queue may be nil.
(And its cousin issue, @sender_thread being nil)
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:
- Construct the
Queuein#initialize - Never set it to
nilor close it (but we may#clearit 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. - Only synchronize when mutating
@sender_thread-> starting it, changing it (when it dies), or setting it tonil(when it finishes due to#stop). Reading@sender_threadfor 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.
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...
Overview
The library is detecting while if a fork happened and it is automatically recreating resources accordingly, which should result in fixing metrics report for applications or frameworks using
forks.This PR is also fixing the telemetry if forks happened.
Technically
Senderis checking if its companion thread is running,SingleThreadSenderis usingProcess.pidMessageBufferis cleaned in order for the new process to not start reporting metrics already handled by the parent processSender/SingleThreadSendermethods thread-safe, it doens't mean that the library is completely thread-safe yet, further work is needed for theForwarderto accomplish this.