Giter VIP home page Giter VIP logo

Comments (13)

MistressRemilia avatar MistressRemilia commented on July 2, 2024 2

@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!).

from crystal.

straight-shoota avatar straight-shoota commented on July 2, 2024 1

@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 ;)

from crystal.

Blacksmoke16 avatar Blacksmoke16 commented on July 2, 2024 1

@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.

from crystal.

MistressRemilia avatar MistressRemilia commented on July 2, 2024 1

@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.

from crystal.

ysbaddaden avatar ysbaddaden commented on July 2, 2024 1

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 😞

@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.

from crystal.

straight-shoota avatar straight-shoota commented on July 2, 2024

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?

from crystal.

MistressRemilia avatar MistressRemilia commented on July 2, 2024

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.

from crystal.

Blacksmoke16 avatar Blacksmoke16 commented on July 2, 2024

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 #14666 as those methods were also in the public API :/

EDIT: And #14367

from crystal.

straight-shoota avatar straight-shoota commented on July 2, 2024

@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.

from crystal.

straight-shoota avatar straight-shoota commented on July 2, 2024

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.

from crystal.

yxhuvud avatar yxhuvud commented on July 2, 2024

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.

from crystal.

ysbaddaden avatar ysbaddaden commented on July 2, 2024

@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?

from crystal.

yxhuvud avatar yxhuvud commented on July 2, 2024

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.

from crystal.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.