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

[ART-380] Fix ArgumentError due to concurrent require and class use #1354

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Feb 5, 2021

If for some reason multiple tracers are initializing concurrently (see customer issue in #1306 or the flaky test in tracer_integration_spec.rb also fixed by this PR), multiple threads can try to run default_statsd_client concurrently.

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

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:

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 requires 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:
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:
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

This gem is quite nifty for exercising flaky tests, so let's keep it
around.
This was causing a race in
`bundle exec rspec --seed 50224 spec/ddtrace/tracer_integration_spec.rb`
exposing the issue in ART-380 where multiple writer background threads
would trigger initialization of metrics.

This does not fix the underlying issue causing
`ArgumentError: wrong number of arguments (given 2, expected 0)`
(see next commit for that).
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
@ivoanjo ivoanjo requested a review from a team February 5, 2021 12:47
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹

@ivoanjo ivoanjo merged commit d6e0e93 into master Feb 5, 2021
@ivoanjo ivoanjo deleted the fix/metrics-statsd-init-flaky branch February 5, 2021 17:21
@github-actions github-actions bot added this to the 0.46.0 milestone Feb 5, 2021
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.

ArgumentError: wrong number of arguments (given 2, expected 0)
2 participants