Skip to content

Workaround wrong delegation in sucker_punch triggering JRuby bug#1370

Merged
ivoanjo merged 1 commit into
masterfrom
fix/workaround-incorrect-delegation-jruby-bug
Feb 17, 2021
Merged

Workaround wrong delegation in sucker_punch triggering JRuby bug#1370
ivoanjo merged 1 commit into
masterfrom
fix/workaround-incorrect-delegation-jruby-bug

Conversation

@ivoanjo

@ivoanjo ivoanjo commented Feb 17, 2021

Copy link
Copy Markdown
Member

The newly-released sucker_punch 3.0.0 gem includes a few changes to support Ruby 3.0 keyword syntax (see brandonhilkert/sucker_punch#235), but it made our current tests fail in JRuby.

Relevant snippet is (from this failed run):

F.E, [2021-02-17T08:12:15.473545 #1277] ERROR -- : Sucker Punch job error for class: '#<Class:0x17abef0f>' args: [:fail, {}]
ArgumentError wrong number of arguments (given 2, expected 0..1)
/app/spec/ddtrace/contrib/sucker_punch/patcher_spec.rb:62:in `perform'
/usr/local/bundle/jruby/2.5.0/gems/sucker_punch-3.0.0/lib/sucker_punch/job.rb:62:in `__run_perform'
/app/lib/ddtrace/contrib/sucker_punch/instrumentation.rb:32:in `block in __run_perform'
/app/lib/ddtrace/contrib/sucker_punch/instrumentation.rb:77:in `block in __with_instrumentation'
/app/lib/ddtrace/tracer.rb:284:in `trace'
/app/lib/ddtrace/contrib/sucker_punch/instrumentation.rb:73:in `__with_instrumentation'
/app/lib/ddtrace/contrib/sucker_punch/instrumentation.rb:21:in `__run_perform'
/usr/local/bundle/jruby/2.5.0/gems/sucker_punch-3.0.0/lib/sucker_punch/job.rb:38:in `block in perform_async'
/usr/local/bundle/jruby/2.5.0/gems/concurrent-ruby-1.1.8/lib/concurrent-ruby/concurrent/executor/java_executor_service.rb:79:in `run'

Failures:

  1) sucker_punch instrumentation failed job should instrument a failed job
     Failure/Error: expect(job_span).to have_error_type('ZeroDivisionError')
       expected Span with error type "ArgumentError" to have error type "ZeroDivisionError"
     # ./spec/ddtrace/contrib/sucker_punch/patcher_spec.rb:100:in `block in <main>'
     # ./spec/ddtrace/contrib/sucker_punch/patcher_spec.rb:23:in `block in <main>'
     # /usr/local/bundle/jruby/2.5.0/gems/webmock-3.11.2/lib/webmock/rspec.rb:37:in `block in <main>'

An unexpected ArgumentError is occuring in JRuby, making our testcase fail (because it was actually trying to cause an exception to see if it was properly traced).

This issue is caused by a bug in sucker_punch's delegation which then triggered a JRuby/TruffleRuby bug. I've reported the bug to the appropriate upstreams (see #1369), and this workaround makes our CI happy again so that we don't need to wait for upstream to react.

Fixes #1369

The newly-released
[sucker_punch](https://github.com/brandonhilkert/sucker_punch) 3.0.0
gem includes a few changes to support Ruby 3.0 keyword syntax
(see brandonhilkert/sucker_punch#235), but it
made our current tests fail in JRuby.

Relevant snippet is (from
[this failed run](https://app.circleci.com/pipelines/github/DataDog/dd-trace-rb/2764/workflows/596c0f11-c7dd-4e89-a36a-78397cc724c6/jobs/112458/parallel-runs/21)):

```
F.E, [2021-02-17T08:12:15.473545 #1277] ERROR -- : Sucker Punch job error for class: '#<Class:0x17abef0f>' args: [:fail, {}]
ArgumentError wrong number of arguments (given 2, expected 0..1)
/app/spec/ddtrace/contrib/sucker_punch/patcher_spec.rb:62:in `perform'
/usr/local/bundle/jruby/2.5.0/gems/sucker_punch-3.0.0/lib/sucker_punch/job.rb:62:in `__run_perform'
/app/lib/ddtrace/contrib/sucker_punch/instrumentation.rb:32:in `block in __run_perform'
/app/lib/ddtrace/contrib/sucker_punch/instrumentation.rb:77:in `block in __with_instrumentation'
/app/lib/ddtrace/tracer.rb:284:in `trace'
/app/lib/ddtrace/contrib/sucker_punch/instrumentation.rb:73:in `__with_instrumentation'
/app/lib/ddtrace/contrib/sucker_punch/instrumentation.rb:21:in `__run_perform'
/usr/local/bundle/jruby/2.5.0/gems/sucker_punch-3.0.0/lib/sucker_punch/job.rb:38:in `block in perform_async'
/usr/local/bundle/jruby/2.5.0/gems/concurrent-ruby-1.1.8/lib/concurrent-ruby/concurrent/executor/java_executor_service.rb:79:in `run'

Failures:

  1) sucker_punch instrumentation failed job should instrument a failed job
     Failure/Error: expect(job_span).to have_error_type('ZeroDivisionError')
       expected Span with error type "ArgumentError" to have error type "ZeroDivisionError"
     # ./spec/ddtrace/contrib/sucker_punch/patcher_spec.rb:100:in `block in <main>'
     # ./spec/ddtrace/contrib/sucker_punch/patcher_spec.rb:23:in `block in <main>'
     # /usr/local/bundle/jruby/2.5.0/gems/webmock-3.11.2/lib/webmock/rspec.rb:37:in `block in <main>'
```

An unexpected `ArgumentError` is occuring in JRuby, making our testcase
fail (because it was actually trying to cause an exception to see if it
was properly traced).

This issue is caused by a bug in sucker_punch's delegation which then
triggered a JRuby/TruffleRuby bug. I've reported the bug to the
appropriate upstreams (see #1369), and this workaround makes our CI
happy again so that we don't need to wait for upstream to react.

Fixes #1369
@ivoanjo ivoanjo requested a review from a team February 17, 2021 15:07
@ivoanjo ivoanjo merged commit 8398dd7 into master Feb 17, 2021
@ivoanjo ivoanjo deleted the fix/workaround-incorrect-delegation-jruby-bug branch February 17, 2021 16:05
@github-actions github-actions Bot added this to the 0.46.0 milestone Feb 17, 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.

JRuby tests broken in CI due to recent upgrade of sucker_punch gem

2 participants