Update error handling for jruby 10#86
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| return [reg[WINDOWS_KEYNAME].to_s, :windows] | ||
| end | ||
| rescue LoadError | ||
| rescue LoadError, Fiddle::DLError |
There was a problem hiding this comment.
This could result in a NameError in environments where Fiddle is not present. We'll need to deal with that. Maybe rescue a superclass and then check the class name?
There was a problem hiding this comment.
Forgive me for not looking up docs (or maybe misunderstanding...) but I thought of that and did this quick test locally:
➜ ruby-cloud-env git:(jruby-10) irb
irb(main):001* def foo
irb(main):002* rescue DoesNotExist
irb(main):003> end
=> :foo
irb(main):004> foo
=> nil
There was a problem hiding this comment.
It doesn't need to evaluate the rescue unless something actually gets raised. But if something does get raised, we'll get an extra NameError:
irb(main):001* begin
irb(main):002* raise "hello"
irb(main):003* rescue DoesNotExist
irb(main):004* puts "rescued"
irb(main):005> end
(irb):3:in '<main>': uninitialized constant DoesNotExist (NameError)
from <internal:kernel>:168:in 'Kernel#loop'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/gems/3.4.0/gems/irb-1.14.3/exe/irb:9:in '<top (required)>'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/bin/irb:25:in 'Kernel#load'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/bin/irb:25:in '<main>'
(irb):2:in '<main>': hello (RuntimeError)
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb/workspace.rb:101:in 'Kernel#eval'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb/workspace.rb:101:in 'IRB::WorkSpace#evaluate'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb/context.rb:631:in 'IRB::Context#evaluate_expression'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb/context.rb:599:in 'IRB::Context#evaluate'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1053:in 'block (2 levels) in IRB::Irb#eval_input'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1365:in 'IRB::Irb#signal_status'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1045:in 'block in IRB::Irb#eval_input'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1124:in 'block in IRB::Irb#each_top_level_statement'
from <internal:kernel>:168:in 'Kernel#loop'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1121:in 'IRB::Irb#each_top_level_statement'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1044:in 'IRB::Irb#eval_input'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1025:in 'block in IRB::Irb#run'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1024:in 'Kernel#catch'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:1024:in 'IRB::Irb#run'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/3.4.0/irb.rb:906:in 'IRB.start'
from /Users/dazuma/.asdf/installs/ruby/3.4.3/lib/ruby/gems/3.4.0/gems/irb-1.14.3/exe/irb:9:in '<top (required)>'
... 2 levels...
There was a problem hiding this comment.
Ah, oops you are right. Thanks
There was a problem hiding this comment.
Looks like it ultimately derives from StandardError https://github.com/ruby/fiddle/blob/eb285e8d85d9501278042580fe96e7bac4ca29a5/ext/fiddle/fiddle.c#L151
So as you suggested (I think) we could rescue that and then handle LoadError as before in addition to safely (not risking name error) see if it is the Fiddle error.
So essentially:
- rescue StandardError
- Determine type
- If type is LoadError of Fiddle error, handle as before. Else raise.
I would be happy to push a commit with that impl if it would be helpful.
Thanks again for looking in to this.
There was a problem hiding this comment.
LoadError is not actually a StandardError (see https://ruby-doc.org/3.4.1/LoadError.html) so maybe rescue LoadError, then separately rescue StandardError with the additional check.
Sorry this got a bit complicated. Just want to make sure the logic is correct, since this library is a core dependency. Thanks!
There was a problem hiding this comment.
Excellent point!
I pushed a commit with that concept. I couldnt figure out how to meaningfully test this in the existing test suite. In leiu of that here is what i did locally:
Original failure:
➜ google_cloud_platform git:(jruby-10) ✗ bundle exec rake test
Coverage may be inaccurate; set the "--debug" command line option, or do JRUBY_OPTS="--debug" or set the "debug.fullTrace=true" option in your .jrubyrc
failed to load native console support: native console on MacOS only supported on i386, x86_64
io/console on JRuby shells out to stty for most operations
Run options: --seed 58871
# Running:
../Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/win32/registry.rb:2: warning: fiddle was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add fiddle to your Gemfile or gemspec to silence this warning.
E.
Finished in 0.051438s, 77.7637 runs/s, 77.7637 assertions/s.
1) Error:
OpenTelemetry::Resource::Detector::GoogleCloudPlatform::.detect#test_0001_returns an empty resource:
Fiddle::DLError: Could not open kernel32.dll
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/fiddle/ffi_backend.rb:492:in 'initialize'
org/jruby/RubyClass.java:1023:in 'new'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/fiddle.rb:93:in 'dlopen'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/fiddle/import.rb:86:in 'block in dlload'
org/jruby/RubyArray.java:2873:in 'collect'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/fiddle/import.rb:77:in 'dlload'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/win32/registry.rb:173:in '<module:Kernel32>'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/win32/registry.rb:171:in '<class:Error>'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/win32/registry.rb:170:in '<class:Registry>'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/win32/registry.rb:74:in '<module:Win32>'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/win32/registry.rb:4:in '<main>'
org/jruby/RubyKernel.java:1183:in 'require'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/bundled_gems.rb:81:in 'block in replace_require'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/gems/shared/gems/google-cloud-env-2.3.0/lib/google/cloud/env/compute_smbios.rb:122:in 'load_product_name'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/gems/shared/gems/google-cloud-env-2.3.0/lib/google/cloud/env/compute_smbios.rb:36:in 'block in initialize'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/gems/shared/gems/google-cloud-env-2.3.0/lib/google/cloud/env/lazy_value.rb:462:in 'perform_compute'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/gems/shared/gems/google-cloud-env-2.3.0/lib/google/cloud/env/lazy_value.rb:279:in 'get'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/gems/shared/gems/google-cloud-env-2.3.0/lib/google/cloud/env/compute_smbios.rb:51:in 'product_name'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/gems/shared/gems/google-cloud-env-2.3.0/lib/google/cloud/env/compute_smbios.rb:83:in 'google_compute?'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/gems/shared/gems/google-cloud-env-2.3.0/lib/google/cloud/env.rb:269:in 'compute_engine?'
lib/opentelemetry/resource/detector/google_cloud_platform.rb:20:in 'detect'
test/opentelemetry/resource/detector/google_cloud_platform_test.rb:31:in 'block in <main>'
org/jruby/RubyBasicObject.java:2701:in 'instance_eval'
org/jruby/RubyBasicObject.java:2729:in 'instance_eval'
4 runs, 4 assertions, 0 failures, 1 errors, 0 skips
Coverage report generated for 098f6bcd4621d373cade4e832627b4f6 to /Users/cas/elastic-repos/opentelemetry-ruby-contrib/resources/google_cloud_platform/coverage.
Line Coverage: 95.74% (45 / 47)
rake aborted!
Command failed with status (1)
org/jruby/ext/monitor/Monitor.java:85:in 'synchronize'
/Users/cas/.rbenv/versions/jruby-10.0.0.1/bin/bundle:25:in '<main>'
Tasks: TOP => test
(See full trace by running task with --trace)
copy the patched file over to the jruby10 env:
➜ google_cloud_platform git:(jruby-10) ✗ cp /Users/cas/elastic-repos/ruby-cloud-env/lib/google/cloud/env/compute_smbios.rb /Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/gems/shared/gems/google-cloud-env-2.3.0/lib/google/cloud/env/compute_smbios.rb
run tests again
➜ google_cloud_platform git:(jruby-10) ✗ bundle exec rake test
Coverage may be inaccurate; set the "--debug" command line option, or do JRUBY_OPTS="--debug" or set the "debug.fullTrace=true" option in your .jrubyrc
failed to load native console support: native console on MacOS only supported on i386, x86_64
io/console on JRuby shells out to stty for most operations
Run options: --seed 30684
# Running:
.../Users/cas/.rbenv/versions/jruby-10.0.0.1/lib/ruby/stdlib/win32/registry.rb:2: warning: fiddle was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add fiddle to your Gemfile or gemspec to silence this warning.
.
Finished in 0.052940s, 75.5570 runs/s, 113.3355 assertions/s.
4 runs, 6 assertions, 0 failures, 0 errors, 0 skips
Coverage report generated for 098f6bcd4621d373cade4e832627b4f6 to /Users/cas/elastic-repos/opentelemetry-ruby-contrib/resources/google_cloud_platform/coverage.
Line Coverage: 95.74% (45 / 47)
There was a problem hiding this comment.
I think this looks good. There were some lingering style (rubocop) errors; I went ahead and pushed a fix for those. (I also pushed a workaround for an unrelated linkinator issue we've been having, to make sure CI gets green.)
With jruby 10 there is a new error type not caught by `LoadError`. This appears to be fallout from this jruby/jruby#8385 refactor. This commit updates the rescue block to handle the existing `LoadError` as well as the `Fiddle::DLError` error. This should bridge compatability between jruby 9 and 10.
This commit updates the `Google::Cloud::Env::ComputeSMBIOS#load_product_name` to handle a new failure method introduced with Jruby10 whereby a `Fiddle::DLError` is raised instead of a `LoadError`. In the case the `Fiddle` gem is not loadable this safely inspects the name of the error by looking up the class name instead of relying on the name `Fiddle::DLError` being defined.
|
@donoghuc Thanks for this! I merged it, and will cut a release at the beginning of next week. |
|
Brilliant. Thanks! |
2 participants
With jruby 10 there is a new error type not caught by
LoadError. This appears to be fallout from this jruby/jruby#8385 refactor. This commit updates the rescue block to handle the existingLoadErroras well as theFiddle::DLErrorerror. This should bridge compatability between jruby 9 and 10.