socketry / async

An awesome asynchronous event-driven reactor for Ruby.
MIT License
2.04k stars 85 forks source link

unable to convert timeout #317

Closed klobuczek closed 3 weeks ago

klobuczek commented 3 weeks ago

After upgrading to async 2.12 I'm getting the following error:

RuntimeError: unable to convert timeout

  0) Session retries read transaction until success
     Failure/Error: Sync { super(*args, **kwargs, &block) }

     RuntimeError:
       unable to convert timeout
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:309:in `select'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:309:in `run_once!'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:283:in `run_once'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:358:in `block in run'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:352:in `handle_interrupt'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:352:in `run'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/kernel/sync.rb:26:in `Sync'
     # ./lib/neo4j/driver/synchronizable.rb:16:in `block (3 levels) in with_sync_wrapper'
     # ./spec/integration/session_spec.rb:77:in `block (2 levels) in <top (required)>'
     # ./spec/support/neo4j_cleaner.rb:12:in `cleaning'
     # ./spec/spec_helper.rb:36:in `block (2 levels) in <top (required)>'

The same spec works with 2.11. Please have a look if this rings a bell or ask me for further troubleshooting. I would be more than happy to help getting to the bottom of it. I'm using ruby 3.2.0.

ioquatix commented 3 weeks ago

Hmm, thanks for this report, I have an idea what is going on.

ioquatix commented 3 weeks ago

I've released io-event v1.6.1 with https://github.com/socketry/io-event/pull/104 which should include what the duration value is. Do you mind updating and running your test again? The error message should be more descriptive.

klobuczek commented 3 weeks ago

I'm including now the entire rspec preamble which I did not include before although the same warnings were present in the previous version as well.

/bin/bash -c "/Users/Heinrich_Klobuczek/.rvm/bin/rvm ruby-3.2.0 do /Users/Heinrich_Klobuczek/.rvm/rubies/ruby-3.2.0/bin/ruby -x /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/bin/bundle exec /Users/Heinrich_Klobuczek/.rvm/rubies/ruby-3.2.0/bin/ruby /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/bin/rspec /Users/Heinrich_Klobuczek/mck/neo4j-ruby-driver/spec/integration/session_spec.rb --require teamcity/spec/runner/formatter/teamcity/formatter --format Spec::Runner::Formatter::TeamcityFormatter"
Testing started at 06:09 ...
/Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/io-event-1.6.2/lib/io/event/support.rb:27: warning: IO::Buffer is experimental and both the Ruby and C interface may change in the future!
Fiber#storage has borked keys and is being monkey-patched.
Run options: exclude {:concurrency=>true, :version=>#<Method: Object(DriverHelper::Helper)#not_version?(requirement) ./spec/support/driver_helper.rb:46>, :auth=>:none}

ArgumentError: unable to convert timeout: 0.7999849999323487 seconds

  0) Session retries read transaction until success
     Failure/Error: Sync { super(*args, **kwargs, &block) }

     ArgumentError:
       unable to convert timeout: 0.7999849999323487 seconds
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:309:in `select'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:309:in `run_once!'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:283:in `run_once'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:358:in `block in run'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:352:in `handle_interrupt'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/async/scheduler.rb:352:in `run'
     # /Users/Heinrich_Klobuczek/.rvm/gems/ruby-3.2.0/gems/async-2.12.0/lib/kernel/sync.rb:26:in `Sync'
     # ./lib/neo4j/driver/synchronizable.rb:16:in `block (3 levels) in with_sync_wrapper'
     # ./spec/integration/session_spec.rb:77:in `block (2 levels) in <top (required)>'
     # ./spec/support/neo4j_cleaner.rb:12:in `cleaning'
     # ./spec/spec_helper.rb:36:in `block (2 levels) in <top (required)>'
ioquatix commented 3 weeks ago

Are you by any chance providing x.seconds as an argument to a timeout function?

klobuczek commented 3 weeks ago

Generally we are dealing with timeouts a lot and we do specify timeouts as ActiveSupport::Duration. However, I don't see at the moment how we would be explicitly passing that to io-event. And that without a conversion.

ioquatix commented 3 weeks ago

I wonder if some how if this is a monkey patch from Rails... Can you share any places you have IO.timeout=, #with_timeout, wait*(timeout) etc.

klobuczek commented 3 weeks ago

we have no single occurrence of the above patterns in our gem code directly

ioquatix commented 3 weeks ago

Can you try io-event v1.6.3 - I've added a fallback to try and convert whatever duration you give it to a float.

klobuczek commented 3 weeks ago

Now the error is a bit different:

TypeError: can't convert ActiveSupport::Duration into Float
klobuczek commented 3 weeks ago

The #to_f method on ActiveSupport::Duration behaves correctly. I wonder if you could use that on any timeout parameter type.

garrett-octopusapp commented 3 weeks ago

I'm getting TypeError: can't convert ActiveSupport::Duration into Float too... But I just picked up the library for the first time, so I don't have any sense of whether it's a regression, or me doing something wrong, or something else.

FWIW though, changing this line to @selector.select(interval.to_f) made my error go away. I don't know if that's a good idea or not, but I think it supports @klobuczek 's thought.

ioquatix commented 3 weeks ago

Thanks for the discussion.

It's important to me that the internal interfaces avoid a lot of type acrobatics to remain on the fast path.

I agree adding @selector.select(interval.to_f) can help, but after sleeping on it, I've come to the conclusion that the problem is not at that point, it's actually Timers#schedule which needs to convert the timeout argument to a float. Otherwise, the priority queue may not sort the values correctly.

This is actually how it was done previously: https://github.com/socketry/timers/blob/039bbd2750d5e50721789ef5d3404b18c36517bc/lib/timers/events.rb#L62

I'll add this to the new implementation and the problem should go away.

klobuczek commented 3 weeks ago

Confirming, tests pass again. @ioquatix thank you for the quick resolution.

ioquatix commented 3 weeks ago

Thanks for reporting the issue!