API breakage with IO::Evented#evented_read

[MistressRemilia]

Crystal v1.12.2 and previous has this for the signature:

def evented_read(slice : Bytes, errno_msg : String, &) : Int32

But the current trunk as of time of writing (commit [a1db18c3b8ca68dca0481b3622c04107321eb5be]) has this signature:

def evented_read(errno_msg : String, &) : Int32

The change is totally understandable (slice is pointless in the older version, it's just yielded again), and is technically not that big of a deal... but the removal breaks the API despite the semantic version major number not increasing, which looks bad and necessitates populating code with silly {% if compare_versions(Crystal::VERSION, "1.13.0") <= 0 %} calls.

[straight-shoota]

evented_read is an internal implementation detail of the standard library. It's not intended to be part of the public API. Dropping the slice parameter is therefore considered an internal refactoring. We're planning to remove this method entirely in the future.

Could you explain what you're using it for?

[MistressRemilia]

It's part of my input loop in Benben, which runs on a separate fiber. The code was written before Benben was changed to use S-Lang for its TUI library, and the separate fiber was required back then in order to keep the program from freezing and waiting for input. While I plan to change to using S-Lang directly for input in a future version in Benben, I don't know when this change will occur (definitely not anytime soon - I'm in a code freeze at the moment).

The API was publicly available, and there are other bits of the stdlib that are public but not documented, so I didn't think it was internal.

[Blacksmoke16]

Yea, going back to 1.0.0, IO::Evented is a public type: https://crystal-lang.org/api/1.0.0/IO/Evented.html so seems we need to keep this overload and deprecate it. Also would need to do something about https://github.com/crystal-lang/crystal/pull/14666 as those methods were also in the public API :/

EDIT: And https://github.com/crystal-lang/crystal/pull/14367

[straight-shoota]

@MistressRemilia I'm quite intrigued to learn about how you came up with the idea to use evented_read this way. It's certainly not adequate usage and entirely pointless.

evented_read is for wrapping low-level primitives such as LibC.read which - in non-blocking mode - indicate via EAGAIN to try again when the file descriptor is readable.

FileDescriptor#read does not do this. In fact it calls evented_read internally. Wrapping an already evented method inside evented_read has no effect. Especially when the block's output vlaue is always 1 (evented_read's functionality is only invoked on an error value of -1). Instead of adding stdlib version detection, you should entirely drop this method call.

[straight-shoota]

@Blacksmoke16 It's unfortunate that IO::Evented shows up in the published API docs. It's a platform-specific module which IMO should never have been part of the public API. Code that uses it would always break on Windows and is correctly not included in the API docs on Windows.

The methods in this module are very low-level features which require an understanding of the event loop details to use them properly (there's no public documentation either). Who ever has this understanding would realize that there's no reason to call them from user code (they're only useful for stdlib implementation and not relevant for user code applications) and be aware of the implications if they still find an esoteric use case ;)

[Blacksmoke16]

@straight-shoota We should at least go back and add the breaking label to those PRs so they're highlighted in the changelog. Also should mention this reasoning in the release notes post on the website.

[MistressRemilia]

@MistressRemilia I'm quite intrigued to learn about how you came up with the idea to use evented_read this way. It's certainly not adequate usage and entirely pointless.

evented_read is for wrapping low-level primitives such as LibC.read which - in non-blocking mode - indicate via EAGAIN to try again when the file descriptor is readable.

FileDescriptor#read does not do this. In fact it calls evented_read internally. Wrapping an already evented method inside evented_read has no effect. Especially when the block's output vlaue is always 1 (evented_read's functionality is only invoked on an error value of -1). Instead of adding stdlib version detection, you should entirely drop this method call.

This is pretty old code (back when it just interfaced raw with the terminal) and so my memory may be a bit fuzzy, but without the evented_read, the program would either hang and wait for input regardless of what I did, or just not receive input at all. One of those two things were the reason. I believe I got this code from example code elsewhere for reading raw unblocking input from the terminal from within a separate fiber and then compared it with my C knowledge to arrive at the final code.

As I said, I do intend to switch to using S-Lang for input in the future, but not for the upcoming v0.5.0 release of Benben at the end of July. Though if I need to postpone my release so I can get this change in and have it be well-tested, I will.

This is the second time I've run into stdlib API breakage, so I wanted to at least report it.

[straight-shoota]

Yeah, thanks for reporting. It's certainly good to talk about it. Now we'll need to figure out what we want to do about it.

I'm pretty sure this would've never had any effect. evented_read is a no-op unless the block output is -1, but in your code it's always 1.

[ysbaddaden]

This is related to the RFC 7 refactor. Both IO::Overlapped and IO::Evented are OS specific implementation details of the event loops that leaked out to the public interface. We already removed IO::Overlapped for Crystal 1.13 and we plan to remove IO::Evented for Crystal 1.14 :disappointed:

@straight-shoota maybe we should undocument IO::Evented for the 1.13 release?

@MistressRemilia I second @straight-shoota's analysis: since #read already calls #evented_read internally, the #read call is already evented and will always have read something or reached EOF, so your manual call is a NOOP. You can safely remove it without any impact or need to refactor.

[yxhuvud]

we plan to remove IO::Evented for Crystal 1.14 😞

Will wait_readable still exist and be usable after that? While it is unfortunately limited to certain platforms, on Linux in particular it is quite common to use pipes and other files to communicate between processes and it is not always that reading directly is possible or wanted (for example, waking up and then invoke a C library function on the file descriptor is something I've seen multiple times in bindings).

wait_readable is very convenient for that use case and works fantastic together with the crystal event loop, so it would be a shame if it was hidden away.

[ysbaddaden]

@yxhuvud We probably won't be able to, since we want to support whatever kind of event loop. While epoll and kqueue could, I'm not sure about io_uring and that won't work with IOCP.

For pipes we can use File since #14255 (released in 1.12?):

File.open("path/to/fifo", blocking: false)

Manual polling, however, isn't in RFC 7. You might want to comment your (valid) use-case there. If you have some ways to implement it, that would be even better. Maybe just a pair of #wait_readable and #wait_writable methods. Probably not compatible with Windows, but maybe it could be an optional EventLoop interface?

[yxhuvud]

For pipes we can use File

I'm fairly certain pipes have worked more or less fine since a very long time. At the very least since before MT as pipes are what is used to send fibers between threads. But that is beside the point - the actual kind of file descriptor doesn't matter all that much for this usecase as long as it has a meaningful definition of being readable and being able to act on that without actually reading.

I'm not sure about io_uring

io-uring supports it no problem. It is literally what the existing (though probably outdated) uring branch is built on.

Probably not compatible with Windows, but maybe it could be an optional EventLoop interface?

I'd be very fine with that as looking at readability/writability as far as I know is not an established way to interact with other things on windows as it is elsewhere. And the APIs that would use that don't work on windows anyhow (not counting WSL).

I did look into if it was possible to implement them on windows, and IIRC, the situation is something like "yeah, readability wait can sortof be implemented in a limited way using private interfaces, and writability is just not possible.

[MistressRemilia]

@MistressRemilia I second @straight-shoota's analysis: since #read already calls #evented_read internally, the #read call is already evented and will always have read something or reached EOF, so your manual call is a NOOP. You can safely remove it without any impact or need to refactor.

I removed the call and yeah, it works, so this is fine enough for now. There was a time it didn't, but it's been long enough that I can't remember why (yay bad memory!).