socketry / async

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

Hanging on swallowed error #161

Closed morgoth closed 1 year ago

morgoth commented 2 years ago

This simple code hangs:

Async do |task|
  task.async do
    # raise TypeError # this properly raises an error
    sleep nil
  end
end.wait
sleep nil
(irb):62:in `sleep': can't convert NilClass into time interval (TypeError)

so when I raise TypeError by hand it properly interrupts and report an error, but hangs on the sleep nil which standalone raises TypeError

bruno- commented 2 years ago

Hanging on swallowed error

Well, the error is not really "swallowed" or suppressed.

Inside an Async block sleep is handled by an Async::Scheduler#kernel_sleep fiber scheduler method. The implementation for kernel_sleep method looks like this:

https://github.com/socketry/async/blob/d1271fdfcb3ebf2376b6b8c0f477ea3bbe36481f/lib/async/scheduler.rb#L146-L152

Note it does not error if nil is passed. Instead, it effectively waits indefinitely.

morgoth commented 2 years ago

hmm, I thought I'm using sleep from Ruby standard lib, so this is unexpected. Also why having a different behaviour than in standard lib? What are other methods that are overwritten? Shouldn't this be documented somewhere?

bruno- commented 2 years ago

I could be wrong, but I think this is not considered a bug. I'm sure you could find more "minor gotchas" like this with other blocking methods that delegate to fiber scheduler.

Fiber scheduler allows you to change the way how some ruby built-in methods work. Commonly, a user explicitly opts-in to use a fiber scheduler, so small differences from built-in methods are fine - user explicitly opted into that.

In the case of async that's maybe less obvious, but you still ARE using a fiber scheduler, so it kinda is a user's choice.

bruno- commented 2 years ago

Here are the docs: https://docs.ruby-lang.org/en/3.1/Fiber/SchedulerInterface.html

ioquatix commented 2 years ago

Thanks @bruno- this is an interesting compatibility issue.

Since sleep(nil) is invalid in Ruby, maybe we could consider introducing this behaviour to make it consistent. Handling zero argument case rather than default argument = nil is less efficient in Pure Ruby than in C.

ioquatix commented 1 year ago

I think sleep() and sleep(nil) should be considered the same, but in CRuby it currently isn't. I don't have a solution. If you write this:

def sleep(time = nil)
  if time
    sleep_for_time(time)
  else
    sleep_forever
  end
end

It's impossible to know if the time was nil because it was the default or the user provided nil. It's hard to replicate the CRuby interface.

ioquatix commented 1 year ago

https://bugs.ruby-lang.org/issues/19094

ioquatix commented 1 year ago

It was accepted by Matz, so this will be fixed in Ruby 3.3: https://github.com/ruby/ruby/pull/7484