Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

io_read and io_write should be optional? #2

Open
singpolyma opened this issue Feb 28, 2022 · 8 comments · May be fixed by #4
Open

io_read and io_write should be optional? #2

singpolyma opened this issue Feb 28, 2022 · 8 comments · May be fixed by #4

Comments

@singpolyma
Copy link
Contributor

It's my understanding that io_read and io_write are not required in the interface and that there is a fallback if they are not present. However because of how the specs work, the expectation that they be called causes them to be defined, which causes them to be called, and then fails with a confusing error. Maybe these specs could define themselves only if the relevant methods are present?

@bruno-
Copy link
Owner

bruno- commented Feb 28, 2022

Hm, I think they're optional only so they work with fiber schedulers written for ruby 3.0 (io_read and io_write were added in 3.1). The official docs for 3.1 don't mention they're optional so I didn't consider making any exceptions...

All that said, I'm open to any specific improvements. Maybe we can differentiate specs for ruby 3.0 and 3.1?

@singpolyma
Copy link
Contributor Author

Hmm. They were optional (and undocumented) in 3.0 and are marked as experimental in 3.1 (having changed their required arguments). I assumed this meant they were still optional and one only ought to implement them if needed (which I assume is for io_uring or something).

It seems that under 3.1 https://github.com/ruby/ruby/blob/ae51f304d28ca8ff3e0680448371c936f10490d0/io_buffer.c#L1905 the mechanism for fallback has changed, but still if the method is undefined it doesn't error but falls through to a default implementation. However that fallback looks quite a lot like it just calls read once and returns the result, so I expect it produces EAGAIN? That said, the current set of specs from this gem pass for me without these methods implemented except for the one spec that expects them to be called.

@bruno-
Copy link
Owner

bruno- commented Mar 1, 2022

I just spent a lot of time investigating this, but I can't make strong conclusions. I found another place where this hook is called:

https://github.com/ruby/ruby/blob/7fe0ebc4e7abd78501094cbb2d47918c8ff29f60/io.c#L1134-L1141

Here's my question regarding io_read and io_write: if these methods are not defined on a scheduler, does Ruby code fall back to synchronous read/write behavior? If yes, I'd consider that an error given that the Interface document practically mandates async behavior.

I'll try to figure this out.

@bruno-
Copy link
Owner

bruno- commented Mar 1, 2022

Btw. I'm curious about your Fiber Scheduler implementation, is it public?

@singpolyma
Copy link
Contributor Author

It's not done yet, but what I have is public. I'm trying to make one that uses EventMachine (only works on the reactor thread of course) for mix-and-match style in existing EM projects: https://git.sr.ht/~singpolyma/em-fiberscheduler

@bruno-
Copy link
Owner

bruno- commented Mar 2, 2022

I asked Samuel Williams about io_read and io_write:

SamuelWilliams | Yeah they are optional but… it means a lot of IO will be blocking so semantically it's a lot better to have it.

bruno | Hm, are there any benefits to making these methods optional, like ruby 3.0 interface compatibility? If I understand you correctly, if io_read and io_write are not defined, then network IO will be synchronous? That would be a MAYOR downside!

SamuelWilliams | We can make it required if it makes sense there is an interface check in scheduler.c

My thoughts on the above:

  • Yes, io_read and io_write are technically not required.
  • Practically, I see no sense in using a fiber scheduler that's missing these methods. If network IO is synchronous, what's the point?

@bruno-
Copy link
Owner

bruno- commented Mar 2, 2022

Do you mind if I add your fiber scheduler to https://github.com/bruno-/fiber_scheduler_list?

@singpolyma
Copy link
Contributor Author

Sure thing. I've just set up the canonical URL at https://git.singpolyma.net/em_fiberscheduler

@singpolyma singpolyma linked a pull request Mar 2, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants