Skip to content

Commit

Permalink
[ART-380] Fix ArgumentError due to concurrent require and class use
Browse files Browse the repository at this point in the history
If for some reason multiple tracers are initializing concurrently
(see customer issue in #1306 or the flaky test in
`tracer_integration_spec.rb` in the previous commit), multiple threads
can try to run `default_statsd_client` concurrently.

Because Ruby `require`s are not atomic (e.g. you can observe classes
that are half-loaded), our conditional

```ruby
require 'datadog/statsd' unless defined?(::Datadog::Statsd)
Datadog::Statsd.new(default_hostname, default_port)
```

could try to create a `Statsd` instance BEFORE the class was fully
defined, leading to
`ArgumentError: wrong number of arguments (given 2, expected 0)` when
calling `initialize`. This error stems from the custom `#initialize`
method not being available yet, so Ruby falls back to
`Object#initialize`, which does not take any argument.

See also:
* <https://slides.com/ianjo/spotting-unsafe-concurrent-ruby-patterns-fullstack-lx#/0/42/0>
* <https://gitlab.com/ivoanjo/unsafe-concurrent-ruby-patterns/-/blob/master/background_require.rb>

The fix in this case is to always call `require`. Calling `require`
repeatedly has little impact -- a given file is only loaded by
`require` the first time AND concurrent `require`s are protected --
only one thread will get to execute the require, will all others wait
until it finishes.

The follow example demonstrates this:

* `lazy-require.rb`:

```ruby
def try_require
  puts "#{Thread.current} Going to require"
  require_relative './lazy-require-sleep-1'
  puts "#{Thread.current} After require"
end

2.times.map { Thread.new { try_require } }.map(&:join)

try_require
```

* `lazy-require-sleep-1.rb`:

```ruby
puts "#{Thread.current} Running require"
sleep 1
puts "#{Thread.current} Require finished"
```

* Output:

```
$ ruby lazy-require.rb
 #<Thread:0x00007fc08b9081e0 lazy-require.rb:7 run> Going to require
 #<Thread:0x00007fc08b90be80 lazy-require.rb:7 run> Going to require
 #<Thread:0x00007fc08b9081e0 lazy-require.rb:7 run> Running require
 #<Thread:0x00007fc08b9081e0 lazy-require.rb:7 run> Require finished
 #<Thread:0x00007fc08b9081e0 lazy-require.rb:7 run> After require
 #<Thread:0x00007fc08b90be80 lazy-require.rb:7 run> After require
 #<Thread:0x00007fc08b85fa90 run> Going to require
 #<Thread:0x00007fc08b85fa90 run> After require
```

Notice how two threads tried to require, but only one actually ran the
code on the other file, while the other was blocked and only woke
up after the `require` finished.

Fixes #1306
  • Loading branch information
ivoanjo committed Feb 5, 2021
1 parent cb34e2b commit 4863e65
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/ddtrace/metrics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def default_port
end

def default_statsd_client
require 'datadog/statsd' unless defined?(::Datadog::Statsd)
require 'datadog/statsd'

# Create a StatsD client that points to the agent.
Datadog::Statsd.new(default_hostname, default_port)
Expand Down

0 comments on commit 4863e65

Please sign in to comment.