digital-fabric / polyphony

Fine-grained concurrency for Ruby
https://www.rubydoc.info/gems/polyphony
MIT License
658 stars 17 forks source link

Please use `rb_io_descriptor` rather than `fptr->fd`. #104

Closed ioquatix closed 1 year ago

ioquatix commented 1 year ago

Can you elaborate on Backend_accept case where it seems like you want to set the file descriptor?

noteflakes commented 1 year ago

See here: https://github.com/digital-fabric/polyphony/blob/d0c60a647dbabc07520c470e00cac80eb7cbfb41/ext/polyphony/backend_io_uring.c#L1002-L1008

ioquatix commented 1 year ago

Thanks - I know how that works, and we have similar code in ext/socket.. I'm just wondering, should we allow for such a code where the underlying file descriptor can be modified?

Just because it was possible before, doesn't mean it was acceptable usage.

My feeling is, we are trying to avoid memory allocations (IO instances). But should we? Doesn't it feel a bit odd to just change the file descriptor?

If we agree it makes sense, then we should introduce rb_io_descriptor_set(VALUE io, int descriptor) or something. Bearing in mind, this won't work well on JRuby or other Ruby implementation that don't have the notion of a file descriptor.

noteflakes commented 1 year ago

It's not really about modifying fd's, more like creating new socket instances.

I just saw this: https://github.com/ruby/ruby/pull/7880

rb_io_open_descriptor(rb_cIO, master_fd, FMODE_READWRITE | FMODE_SYNC | FMODE_DUPLEX, master_path, RUBY_IO_TIMEOUT_DEFAULT, NULL);

Would that work for creating sockets?

ioquatix commented 1 year ago

Yes, that's the plan.

noteflakes commented 1 year ago

Cool! Thanks for keeping me in the loop!

ioquatix commented 1 year ago

No problem, I really love what you are doing and you are an important consumer of the interfaces I'm working on, so I want to ensure we are aligned.