Skip to content

Commit

Permalink
Mark ddtrace threads as fork-safe
Browse files Browse the repository at this point in the history
**What does this PR do?**

This PR applies to all relevant ddtrace threads a pattern pioneered by
rails: set the `:fork_safe` thread variable to indicate that it's OK
if ddtrace threads are started before a Ruby app calls fork.

**Motivation:**

This PR fixes the issue reported in DataDog#3203. The puma webserver in
clustering mode has a check where it will warn when it detects that
threads were started before the webserver has fork'd the child
processes, see:

https://github.com/puma/puma/blob/32e011ab9e029c757823efb068358ed255fb7ef4/lib/puma/cluster.rb#L353-L359

This is not a problem for ddtrace (as discussed in DataDog#3203), but the
warning is annoying.

While looking into the issue again today, I spotted that actually
there's a way to tell puma that a thread is fine and is it's not a
problem if it's started before the webserver has fork'd: puma
checks if the thread has a `:fork_safe` thread-variable set to `true`.

**Additional Notes:**

We have a bunch of places in ddtrace where we create threads...
I briefly played with the idea of creating a superclass of all
ddtrace Threads, but decided to not get into a bigger refactoring
for such a small issue. Maybe next time...?

**How to test the change?**

Test coverage included. Furthermore, you can try running
a Ruby webapp in puma before/after the change to confirm the
warning is gone.

Fixes DataDog#3203
  • Loading branch information
ivoanjo committed Nov 23, 2023
1 parent 884b3b1 commit e997632
Show file tree
Hide file tree
Showing 10 changed files with 37 additions and 2 deletions.
1 change: 1 addition & 0 deletions lib/datadog/core/remote/worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def start

thread = Thread.new { poll(@interval) }
thread.name = self.class.name unless Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.3')
thread.thread_variable_set(:fork_safe, true)
@thr = thread

@started = true
Expand Down
1 change: 1 addition & 0 deletions lib/datadog/core/workers/async.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ def start_worker
# rubocop:enable Lint/RescueException
end
@worker.name = self.class.name unless Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.3')
@worker.thread_variable_set(:fork_safe, true)

nil
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ def start(on_failure_proc: nil)
end
end
@worker_thread.name = self.class.name # Repeated from above to make sure thread gets named asap
@worker_thread.thread_variable_set(:fork_safe, true)
end

true
Expand Down
1 change: 1 addition & 0 deletions lib/datadog/profiling/collectors/idle_sampling_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ def start
end
end
@worker_thread.name = self.class.name # Repeated from above to make sure thread gets named asap
@worker_thread.thread_variable_set(:fork_safe, true)
end

true
Expand Down
1 change: 1 addition & 0 deletions lib/datadog/tracing/workers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ def start
Datadog.logger.debug { "Starting thread for: #{self}" }
@worker = Thread.new { perform }
@worker.name = self.class.name unless Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.3')
@worker.thread_variable_set(:fork_safe, true)

nil
end
Expand Down
6 changes: 6 additions & 0 deletions spec/datadog/core/remote/worker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@
expect(Thread.list.map(&:name)).to include(described_class.to_s)
end
end

it 'marks the worker thread as fork-safe (to avoid fork-safety warnings in webservers)' do
worker.start

expect(worker.instance_variable_get(:@thr).thread_variable_get(:fork_safe)).to be true
end
end

describe '#stop' do
Expand Down
8 changes: 7 additions & 1 deletion spec/datadog/core/workers/async_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@
end
end

describe 'thread naming' do
describe 'thread naming and fork-safety marker' do
after { worker.terminate }

context 'on Ruby < 2.3' do
Expand Down Expand Up @@ -488,6 +488,12 @@
expect(worker.send(:worker).name).to eq worker_class.to_s
end
end

it 'marks the worker thread as fork-safe (to avoid fork-safety warnings in webservers)' do
worker.perform

expect(worker.send(:worker).thread_variable_get(:fork_safe)).to be true
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@
expect(Thread.list.map(&:name)).to include(described_class.name)
end

it 'marks the new thread as fork-safe' do
start

expect(cpu_and_wall_time_worker.instance_variable_get(:@worker_thread).thread_variable_get(:fork_safe)).to be true
end

it 'does not create a second thread if start is called again' do
start

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@
start
end

it 'marks the new thread as fork-safe' do
start

expect(idle_sampling_helper.instance_variable_get(:@worker_thread).thread_variable_get(:fork_safe)).to be true
end

it 'does not create a second thread if start is called again' do
start

Expand Down
8 changes: 7 additions & 1 deletion spec/datadog/tracing/workers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
end
end

describe 'thread naming' do
describe 'thread naming and fork-safety marker' do
context 'on Ruby < 2.3' do
before do
skip 'Only applies to old Rubies' if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.3')
Expand All @@ -65,6 +65,12 @@
expect(worker.instance_variable_get(:@worker).name).to eq described_class.name
end
end

it 'marks the worker thread as fork-safe (to avoid fork-safety warnings in webservers)' do
worker.start

expect(worker.instance_variable_get(:@worker).thread_variable_get(:fork_safe)).to be true
end
end

describe '#start' do
Expand Down

0 comments on commit e997632

Please sign in to comment.