httprb / http

HTTP (The Gem! a.k.a. http.rb) - a fast Ruby HTTP client with a chainable API, streaming support, and timeouts
MIT License
3.01k stars 321 forks source link

LocalJumpError when upgrading to 5.0 gem #673

Open jgreben opened 3 years ago

jgreben commented 3 years ago

Hi I noticed when updating the gem to 5.0 causes a LocalJumpError when I run my rspec tests. (I have not been able to test it yet while running the app for various other reasons, but will try to do so as soon as possible).

Failure/Error:
         HTTP
           .use(instrumentation: { instrumenter: ActiveSupport::Notifications.instrumenter, namespace: 'symphony' })
           .headers(default_headers.merge(headers))
           .request(method, base_url + path, **other)

       LocalJumpError:
         no block given (yield)
       # /Users/jgreben/.rvm/gems/ruby-2.6.3/gems/http-5.0.0/lib/http/features/instrumentation.rb:32:in `wrap_request'
       # /Users/jgreben/.rvm/gems/ruby-2.6.3/gems/http-5.0.0/lib/http/client.rb:56:in `block in build_request'
       # /Users/jgreben/.rvm/gems/ruby-2.6.3/gems/http-5.0.0/lib/http/client.rb:55:in `each'
       # /Users/jgreben/.rvm/gems/ruby-2.6.3/gems/http-5.0.0/lib/http/client.rb:55:in `inject'
       # /Users/jgreben/.rvm/gems/ruby-2.6.3/gems/http-5.0.0/lib/http/client.rb:55:in `build_request'
       # /Users/jgreben/.rvm/gems/ruby-2.6.3/gems/http-5.0.0/lib/http/client.rb:30:in `request'
       # ./app/services/symphony_client.rb:215:in `request'
       # ./app/services/symphony_client.rb:38:in `session_token'
       # ./app/services/symphony_client.rb:12:in `ping'
       # ./app/controllers/application_controller.rb:41:in `check_unavailable'
tarcieri commented 3 years ago

It looks like ActiveSupport::Notifications::Instrumenter#instrument is expecting a block:

https://api.rubyonrails.org/classes/ActiveSupport/Notifications/Instrumenter.html#method-i-instrument

...but http.rb is not passing one:

https://github.com/httprb/http/blob/4244843/lib/http/features/instrumentation.rb#L32

cc @paul

Edit: well this is even more confusing... it looks like it should only be calling yield if a block was given:

https://github.com/rails/rails/blob/75ac626c4e21129d8296d4206a1960563cc3d4aa/activesupport/lib/active_support/notifications/instrumenter.rb#L24

paul commented 3 years ago

@jgreben Are you able try on Ruby 2.7.x or 3.x, see if you get different results? I wonder if its a 2.6 thing... Also, what version of ActiveSupport is it using?

jgreben commented 3 years ago

ActiveSupport is 5.2.6. I upgraded ruby to 2.7.3 and the LocalJumpError was still there. Unfortunately I am not able to use ruby 3 without resolving a bunch of dependencies and conflicts. Thanks!

paul commented 3 years ago

Ah OK. Looks like Rails 5.2 doesn't do the if block_given? part. https://github.com/rails/rails/blob/v5.2.6/activesupport/lib/active_support/notifications/instrumenter.rb#L23

Looks like this was broken by a rubocop cleanup? https://github.com/httprb/http/commit/f4fb3369d030520e981eb891fb5dd25f8e0403d2#diff-bf40ca1851dc97b2ef1fd9b85e44d722ad130f7042df0d8c1c12738d2b44b873L32 cc @ixti