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

Add EventLoop::FileDescriptor module #14639

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented May 28, 2024

Extracts the system interface for FileDescriptor operations on the event loop to a EventLoop module.

This implements part of crystal-lang/rfcs#7

Related: #14643 does the same for Socket

# Reads at least one byte from the file descriptor into *slice* and continues
# fiber when the read is complete.
# Returns the number of bytes read.
abstract def read(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: maybe explicit both the blocking and EOF behaviors?

Reads at least one byte from the file descriptor into slice.
Blocks the current fiber if no data is available for reading, otherwise returns immediately.
Returns the number of bytes read (up to slice.size).
Returns 0 when EOF is reached.

Copy link
Member Author

@straight-shoota straight-shoota May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that's better 👍

Btw. these descriptions have been up for review in crystal-lang/rfcs#7 😏
Maybe you want to take a look at the other there ones as well?

# Writes at least one byte from *slice* to the file descriptor and continues
# fiber when the write is complete.
# Returns the number of bytes written.
abstract def write(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: same and "when the write is complete" can be ambiguous (writes fully?)

Writes at least one byte from slice to the file descriptor.
Blocks the current fiber if the file descriptor isn't ready for writing, otherwise returns immediately.
Returns the number of bytes written (up to slice.size).

@@ -131,14 +111,14 @@ module Crystal::System::FileDescriptor
# Mark the handle open, since we had to have dup'd a live handle.
@closed = false

evented_reopen
event_loop.close(self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to propose to remove IO::Evented#evented_reopen, but then I realize this is part of the public API somehow. Maybe deprecate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point. Actually, all evented_* methods are documented API: https://crystal-lang.org/api/1.12.1/IO/Evented.html 🤦
I'm pretty sure this is totally unintended. Nobody should need to call these methods directly from user code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is what I mean with "public". I guess we can deprecate all of it...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually, everything from IO::Evented has to be removed, because it's event-loop specific implementation. These methods will need to be moved to the event loop implementation. Same for IO::Overlapped (which is thankfully nodoc).

@beta-ziliani
Copy link
Member

The methods in the wasi interface are new, should they be tested?

@straight-shoota
Copy link
Member Author

@beta-ziliani Not sure what you mean which methods would need to be tested on Wasi?
This isn't really adding any new behaviour, it's just shifting implementations from one class to another.

@beta-ziliani
Copy link
Member

I guess I'm missing the part in which WASI uses unix/file_descriptor.cr. But in this case, doesn't it make sense to abstract these two instead of copying it?

@straight-shoota
Copy link
Member Author

straight-shoota commented May 30, 2024

WASI event loop and libevent are completely different things.
I suppose we could drop the evented wrapper from the WASI implementation, because it does not support non-blocking operations anyway. I left it in for now because evented_read etc. also do error handling. This will be another refactor after this and #14643 are merged.

@beta-ziliani beta-ziliani added this to the 1.13.0 milestone Jun 4, 2024
@straight-shoota straight-shoota merged commit 239032e into crystal-lang:master Jun 5, 2024
61 checks passed
@straight-shoota straight-shoota deleted the refactor/event_loop-file_descriptor branch June 5, 2024 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants