Project

General

Profile

Actions

Feature #18566

closed

Merge `io-wait` and `io-nonblock` gems into core IO

Added by byroot (Jean Boussier) almost 3 years ago. Updated over 2 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:107430]

Description

I think we should reconsider status of io-wait, and consider simply merging it into core's IO.

According to @nobu (Nobuyoshi Nakada) it's only a gem for "historical" reasons.

Any non trivial IO code will likely make use of it, and it's just 400 lines of code.

Recently with the extraction of net-protocol, it was added add a dependency and that caused Ruby 3.1 compatibility issues with some gems (e.g. with mail).

Proposal

  • Merge io-wait into io.c for Ruby 3.2
  • Remove io-wait as a dependency of all gems maintained by ruby-core (e.g. net-protocol).
  • Publish a new io-wait version that is simply an empty gem.
  • Add a lib/io/wait.rb stub, with eventually a deprecation warning.

cc @Eregon (Benoit Daloze) @headius (Charles Nutter) @mame (Yusuke Endoh) @ioquatix (Samuel Williams)


Related issues 3 (1 open2 closed)

Related to Ruby master - Bug #18567: Depending on default gems in stdlib gems when not needed considered harmfulClosedhsbt (Hiroshi SHIBATA)Actions
Related to Ruby master - Feature #18655: Copy `IO#wait_readable`, `IO#wait_writable`, `IO#wait_priority` and `IO#wait` into core.ClosedActions
Related to Ruby master - Feature #18668: Merge `io-nonblock` gems into coreOpenActions

Updated by ioquatix (Samuel Williams) almost 3 years ago

I am happy to do the initial implementation.

Updated by retro (Josef Šimánek) almost 3 years ago

It makes sense to me.

Actions #3

Updated by Eregon (Benoit Daloze) almost 3 years ago

  • Related to Bug #18567: Depending on default gems in stdlib gems when not needed considered harmful added

Updated by Eregon (Benoit Daloze) almost 3 years ago

I agree this would make a lot of sense, also for io-nonblock which is also just a couple IO methods.

Updated by normalperson (Eric Wong) over 2 years ago

I agree this would make a lot of sense, also for io-nonblock which is also just a couple IO methods.

Agree, please remove tiny .so since they're frequently needed.

Ulrich Drepper (former glibc maintainer) states:

Every DSO used increases resource usage. As the DSO How-To
points out, loading a DSO isn't free. There is usually
measurably more memory used. The calls into the DSO are more
expensive than normal function calls. And a lot more.

Link: https://udrepper.livejournal.com/8790.html

I would honestly love if more ext (mainly socket, but also fcntl, etc)
were in core, too.

However, I do suggest keeping empty placeholder .rb files
indefinitely to avoid breakage when distros update ruby. Some
users have scripts in ~/bin running unmodified going back over
20 years.

Updated by byroot (Jean Boussier) over 2 years ago

@normalperson (Eric Wong): answering here since the dev-meeting issue isn't the right place for discussion:

Please just keep lib/io/wait.rb stub empty indefinitely. No need for deprecation warning

I probably wasn't clear, but I meant to use a "verbose warning" meaning only users with -w 1 or $VERBOSE = true would see it, allowing them to realize this require is no longer needed.

Actions #7

Updated by nobu (Nobuyoshi Nakada) over 2 years ago

That warning will be just a noice, for users of a library using io-wait.
It should be only when Warning[:deprecate] is true.

Actions #8

Updated by byroot (Jean Boussier) over 2 years ago

It should be only when Warning[:deprecate] is true.

Yes, that's what I was trying to say.

Updated by hsbt (Hiroshi SHIBATA) over 2 years ago

Remove io-wait as a dependency of all gems maintained by ruby-core (e.g. net-protocol).

It's already done.

Updated by hsbt (Hiroshi SHIBATA) over 2 years ago

I think we should only add require_ruby_version = "< 3.2" to io-wait gem after merging io-wait into core IO class. I'm still not sure why reason to make io/wait.rb to stub file in the future.

Actions #11

Updated by byroot (Jean Boussier) over 2 years ago

  • Subject changed from Merge `io-wait` gem into core IO to Merge `io-wait` and `io-nonblock` gems into core IO

Updated by byroot (Jean Boussier) over 2 years ago

I think we should only add require_ruby_version = "< 3.2" to io-wait gem after merging io-wait into core IO class

I believe this would cause unnecessary disruption at release time. Some gems will have transitive dependencies on io-wait and it may take months for them to update. IMO making the gem empty is better.

I'm still not sure why reason to make io/wait.rb to stub file in the future.

Because lots of code have require "io/wait", so having this stub would prevent hard breakage.

Updated by Eregon (Benoit Daloze) over 2 years ago

I think the gem could just be empty, and so require "io/wait" would just require the stdlib (or noop if a Ruby impl implements it in core).
I wouldn't set require_ruby_version, that dummy io-wait gem would work for all Ruby versions with a io/wait stdlib, so all Ruby versions.
There is some more discussion about the dummy gem in https://github.com/ruby/io-wait/issues/12

Updated by deivid (David Rodríguez) over 2 years ago

I think an empty gem is the strategy the ruby2_keywords gem has already followed on Ruby 3.1 and it worked fine (al least nobody has complained).

Updated by deivid (David Rodríguez) over 2 years ago

Actually not exactly, while ruby2_keywords ships empty with ruby 3.1, manually installing it does bring a .rb file.

Updated by mame (Yusuke Endoh) over 2 years ago

This issue was discussed at the dev meeting.

The motivating issue of this proposal (the dependency issue of io-wait / net-protocol / net-smtp) is already solved by removing the dependency from net-protocol to io-wait. If so, there is no strong reason to make io-wait built-in.

This does not mean we should keep it as a separate gem. It is possible to make it built-in. However, @matz (Yukihiro Matsumoto) said that some methods in io-wait are very arguable to make them built-in.

  • wait_readable and wait_writable are maybe okay.
  • No attendee knows what wait_priority is.
  • The name ready? sounds weird because it does not say "what is ready". Maybe renaming is needed. (ready_to_read? or what not?)
  • nread is clearly a very bad name. The use case is neither clear. @nobu (Nobuyoshi Nakada), who introduced the method, said that he himself regretted its introduction.

It would be good to create a ticket and discuss for each method, rather than making the gem entirely built-in. After some methods are built-in (with different names), io-wait gem should remain to keep aliases for compatibility.

@akr (Akira Tanaka) was against to make io/nonblock built-in because it is a old-style interface for nonblock IOs. He recommended IO#read_nonblock and IO#write_nonblock instead of the gem.

Updated by ioquatix (Samuel Williams) over 2 years ago

wait_readable and wait_writable are maybe okay.

No attendee knows what wait_priority is.

For reading MSG_OOB data.

The name ready? sounds weird because it does not say "what is ready". Maybe renaming is needed. (ready_to_read? or what not?)

Some of this method should be deprecated/removed IMHO.

nread is clearly a very bad name. The use case is neither clear. nobu (Nobuyoshi Nakada), who introduced the method, said that he himself regretted its introduction.

Agree.

akr (Akira Tanaka) was against to make io/nonblock built-in because it is a old-style interface for nonblock IOs. He recommended IO#read_nonblock and IO#write_nonblock instead of the gem.

There are a lot of use cases for wait_readable (accept, connect etc) and probably some for wait_writable (connect?). It's not just for reading and writing.

Updated by byroot (Jean Boussier) over 2 years ago

My personal opinion is that wait_readable and wait_writable should be in core IO. All the rest can stay in the gem.

I'll try to submit a PR and feature request for that soon.

Updated by Eregon (Benoit Daloze) over 2 years ago

Those methods have been in stdlib for a while, I don't think moving them to core is an issue (their name and behavior is established already).
And I think renaming is not on the table either for compatibility (too high effort/too low gain).

What's the problem with nread? It's "how many bytes are available to read", seems fairly obvious. It's not the best name, but that's hardly the only method like that (e.g. byterindex is no better I'd say).
FWIW Java has a similar method: https://docs.oracle.com/javase/7/docs/api/java/io/InputStream.html#available()

Re ready? the docs make it clear it's about input so reading (can always improve the docs).

wait_priority just needs better docs, that seems clear.

I think we shouldn't keep any methods in the io-wait gem.
Those methods need lots of IO internals (e.g., the C ext includes ruby/io.h, which exposes all internal details of rb_io_t), and I think it makes no sense to expose the IO internals of all Ruby implementations in that gem just for "not being in core".
Users don't care, if they need these methods they will require it and use them, core or not does not change much for them, but it does a lot for Ruby implementations.
As an example, TruffleRuby does not want to expose IO internals, so it remains free to refactor those as needed.

Those 2 gems (io-wait and io-nonblock) seem extremely unlikely to ever be removed from stdlib/default gems, because they are already needed by many programs.
That means for TruffleRuby we need to either to use the C extension for those gems (but that's a significant overhead for just these few methods), or we implement them in Ruby using TruffleRuby Primitives and then it does not make sense to have that code in a gem (would be exposing internals subject to change).

Another options is to move those gems back to plain stdlib (not a gem). These 2 seem to have very few benefits to be gems, and have many disadvantages due to needing IO internals.
These 2 gems are essential methods to deal with IO, having them as gems is just causing problems IMHO.

Actions #20

Updated by Eregon (Benoit Daloze) over 2 years ago

mame (Yusuke Endoh) wrote in #note-16:

@akr (Akira Tanaka) was against to make io/nonblock built-in because it is a old-style interface for nonblock IOs. He recommended IO#read_nonblock and IO#write_nonblock instead of the gem.

io/nonblock is useful to tell if an IO is in non-blocking mode and to set it upfront.
This is needed when using a Fiber scheduler but also other cases.
In fact https://bugs.ruby-lang.org/issues/18630#note-9 is another case, read_nonblock/write_nonblock are not useful for that case but checking/setting whether an IO is nonblocking is needed.

In fact io/nonblock is so small it's already core in TruffleRuby.
Many core IO methods even need to check/set whether an IO is nonblocking, so it's natural to just use the existing methods for that when such IO methods are written in Ruby.

Actions #21

Updated by byroot (Jean Boussier) over 2 years ago

  • Related to Feature #18655: Copy `IO#wait_readable`, `IO#wait_writable`, `IO#wait_priority` and `IO#wait` into core. added

Updated by byroot (Jean Boussier) over 2 years ago

  • Status changed from Open to Closed

Closing in favor of [Feature #18655], since wait_readable and wait_writable are really the two method I personally care about.

I suggest people who feel strongly about other methods from io-wait / io-nonblock open individual feature requests for each method or group of method that they think should be merged.

Updated by ioquatix (Samuel Williams) over 2 years ago

The core method for waiting is IO#wait. Everything else if I recall correctly is just a wrapper around that core method. So, while I don't have a strong opinion about introducing IO#wait_readable and IO#wait_writable (it's true they are popular interface), I would suggest that the most important method is IO#wait which should be introduced.

As io/nonblock has clunky interface for IO#wait, I suggest we implement subset in core Ruby and if io/wait is required, it implements the old clunky interface (super set of core IO#wait).

Actions #24

Updated by Eregon (Benoit Daloze) over 2 years ago

Updated by ioquatix (Samuel Williams) over 2 years ago

We've moved #wait, #wait_readable, #wait_writable, #wait_priority into core.

I'll make a PR for io-nonblock gem and we can discuss in more detail.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0