bruno- / fiber_scheduler

Ruby's missing Fiber Scheduler implementation.
MIT License
119 stars 5 forks source link

Question about io_read #1

Open singpolyma opened 2 years ago

singpolyma commented 2 years ago

I have a question about the logic in io_read at https://github.com/bruno-/fiber_scheduler/blob/main/lib/fiber_scheduler/selector.rb#L98

It seems that it considers the resource unavailable (-EAGAIN) if length == 0 and the read failed. This makes sense because if I try to read nothing and it fails, then it must not be possible to read. However the read does not use length at all! It uses maximum_size which looks likely to be related to length, but is not the same value. Should this perhaps instead says if maximum_size > 0 ?

bruno- commented 2 years ago

Hey,

the #io_read method does not have a conventional read-like behavior. Check the docs for io_read, I think they will make everything clear.

https://docs.ruby-lang.org/en/3.1/Fiber/SchedulerInterface.html#method-i-io_read

singpolyma commented 2 years ago

So, reading over that again, the issue I see is that if one calls IO#read when the fd has nothing ready at all, then io.read_nonblock will return :wait_readable and if length was zero then the code will return a hard failure -EAGAIN instead of waiting for something to become available.

In practise, copying this code into my fiber scheduler I see the above happen when running the specs, which causes them to fail.

bruno- commented 2 years ago

Yea, this is interesting.

So, the selector from this gem was taken from Samuel's io-event gem and slightly changed. I still put his copyright at the top of the file, because it's mostly Samuel's work.

The length > 0 branches were explicitly added to io-event just a few days before ruby 3.1 was released, see commit:

https://github.com/socketry/io-event/commit/f173fadeeeb4439678f5725a96a6756486d5085f#diff-a12eb715e8d43c5484492317b228fc02363668ad6c4ac27e7a3e0f94cb26da18

bruno- commented 2 years ago

It's obviously a special case for something, I'm not sure what.

We should ask Samuel - can I ask him to add you to socketry slack?

singpolyma commented 2 years ago

Sure, you can find my email for such invites, etc at https://singpolyma.net/

taq commented 1 year ago

Hey @bruno- !

Just checked that, with the basic example:

require "fiber_scheduler"
require "open-uri"

FiberScheduler do
  Fiber.schedule do
    URI.open("https://httpbin.org/delay/2")
  end

  Fiber.schedule do
    URI.open("https://httpbin.org/delay/2")
  end
end

Running with 3.1.0, it works ok, but with 3.2.0:

fiber_scheduler-0.13.0/lib/fiber_scheduler.rb:175:in `io_read': wrong number of arguments (given 4, expected 3) (ArgumentError)

Seems something changed with 3.2.0.

aristotelesbr commented 1 year ago

+1 @taq

Here it works if you add (:waiting) to the block. 🤷🏻‍♂️


require "fiber_scheduler"
require "open-uri"

FiberScheduler do
  Fiber.schedule(:waiting) do
    URI.open("https://httpbin.org/delay/2")
  end
end
taq commented 1 year ago

Nice @aristotelesbr !

Will try here and take a look why on 3.2 it is needed.

kingdonb commented 1 year ago

Can someone please elaborate on what Fiber.schedule(:waiting) is going to change, I blindly applied this fix to my fibers app to try to get it to run on 3.2.1

And while it does run with this change, it does not "run"

The fibers are not firing anymore. Is there a corollary command to run later, which should start the fibers that are waiting?

(I'm rolling back to Ruby 3.1.3 for now, to get the fibers running again)

I'm sure I can find more information about this myself searching, but it's the third time I've had to come back to this thread so I thought it might actually be helpful to ask for clarification at this point!

edit: Code https://github.com/kingdonb/simplest-commitbee/pull/25

taq commented 1 year ago

Hey @kingdonb

I still didn't have time to take a look at this, but seems some method calls changed on 3.2. I noticed it prior to give a Ruby online class where I showed Fibers.

taq commented 1 year ago

Hey @bruno-

I checked where the error is, and it's here on rvm/gems/ruby-3.2.0/gems/fiber_scheduler-0.13.0/lib/fiber_scheduler.rb:175:

  def io_read(io, buffer, length)
    @selector.io_read(Fiber.current, io, buffer, length)
  end

but the current code here on Github on the same file is

def io_read(fiber, io, buffer, length, offset = 0)

which handle the offset as the last parameter, making it work with Ruby 3.2.0 as the commit here provides: https://github.com/bruno-/fiber_scheduler/commit/316d5aa4bafddca197c4cd3bd5e77a99ab11c184

So, seems we just need a new gem release. :-)

taq commented 1 year ago

@bruno- if you need some help, just let me know.