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

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/crystal/system/event_loop.cr
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ abstract class Crystal::EventLoop
end
end

require "./event_loop/file_descriptor"

abstract class Crystal::EventLoop
# The FileDescriptor interface is always needed, so we include it right in
# the main interface.
include FileDescriptor
end

{% if flag?(:wasi) %}
require "./wasi/event_loop"
{% elsif flag?(:unix) %}
Expand Down
16 changes: 16 additions & 0 deletions src/crystal/system/event_loop/file_descriptor.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
abstract class Crystal::EventLoop
module FileDescriptor
# 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).


# Closes the file descriptor resource.
abstract def close(file_descriptor : Crystal::System::FileDescriptor) : Nil
end
end
12 changes: 10 additions & 2 deletions src/crystal/system/file_descriptor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,17 @@ module Crystal::System::FileDescriptor
# cooked mode otherwise.
# private def system_raw(enable : Bool, & : ->)

# private def system_read(slice : Bool) : Int32
private def system_read(slice : Bytes) : Int32
event_loop.read(self, slice)
end

# private def system_write(slice : Bool) : Int32
private def system_write(slice : Bytes) : Int32
event_loop.write(self, slice)
end

private def event_loop : Crystal::EventLoop::FileDescriptor
Crystal::EventLoop.current
end
end

{% if flag?(:wasi) %}
Expand Down
24 changes: 24 additions & 0 deletions src/crystal/system/unix/event_loop_libevent.cr
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,28 @@ class Crystal::LibEvent::EventLoop < Crystal::EventLoop
end
end
end

def read(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
file_descriptor.evented_read("Error reading file_descriptor") do
LibC.read(file_descriptor.fd, slice, slice.size).tap do |return_code|
if return_code == -1 && Errno.value == Errno::EBADF
raise IO::Error.new "File not open for reading", target: file_descriptor
end
end
end
end

def write(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
file_descriptor.evented_write("Error writing file_descriptor") do
LibC.write(file_descriptor.fd, slice, slice.size).tap do |return_code|
if return_code == -1 && Errno.value == Errno::EBADF
raise IO::Error.new "File not open for writing", target: file_descriptor
end
end
end
end

def close(file_descriptor : Crystal::System::FileDescriptor) : Nil
file_descriptor.evented_close
end
end
24 changes: 2 additions & 22 deletions src/crystal/system/unix/file_descriptor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,6 @@ module Crystal::System::FileDescriptor
STDOUT_HANDLE = 1
STDERR_HANDLE = 2

private def system_read(slice : Bytes) : Int32
evented_read("Error reading file") do
LibC.read(fd, slice, slice.size).tap do |return_code|
if return_code == -1 && Errno.value == Errno::EBADF
raise IO::Error.new "File not open for reading", target: self
end
end
end
end

private def system_write(slice : Bytes) : Int32
evented_write("Error writing file") do
LibC.write(fd, slice, slice.size).tap do |return_code|
if return_code == -1 && Errno.value == Errno::EBADF
raise IO::Error.new "File not open for writing", target: self
end
end
end
end

private def system_blocking?
flags = fcntl(LibC::F_GETFL)
!flags.bits_set? LibC::O_NONBLOCK
Expand Down Expand Up @@ -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).

end

private def system_close
# Perform libevent cleanup before LibC.close.
# Using a file descriptor after it has been closed is never defined and can
# always lead to undefined results. This is not specific to libevent.
evented_close
event_loop.close(self)

file_descriptor_close
end
Expand Down
24 changes: 24 additions & 0 deletions src/crystal/system/wasi/event_loop.cr
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,30 @@ class Crystal::Wasi::EventLoop < Crystal::EventLoop
def create_fd_read_event(io : IO::Evented, edge_triggered : Bool = false) : Crystal::EventLoop::Event
raise NotImplementedError.new("Crystal::Wasi::EventLoop.create_fd_read_event")
end

def read(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
file_descriptor.evented_read("Error reading file_descriptor") do
LibC.read(file_descriptor.fd, slice, slice.size).tap do |return_code|
if return_code == -1 && Errno.value == Errno::EBADF
raise IO::Error.new "File not open for reading", target: file_descriptor
end
end
end
end

def write(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
file_descriptor.evented_write("Error writing file_descriptor") do
LibC.write(file_descriptor.fd, slice, slice.size).tap do |return_code|
if return_code == -1 && Errno.value == Errno::EBADF
raise IO::Error.new "File not open for writing", target: file_descriptor
end
end
end
end

def close(file_descriptor : Crystal::System::FileDescriptor) : Nil
file_descriptor.evented_close
end
end

struct Crystal::Wasi::Event
Expand Down
21 changes: 21 additions & 0 deletions src/crystal/system/win32/event_loop_iocp.cr
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,27 @@ class Crystal::Iocp::EventLoop < Crystal::EventLoop
def create_timeout_event(fiber) : Crystal::EventLoop::Event
Crystal::Iocp::Event.new(fiber, timeout: true)
end

def read(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
handle = file_descriptor.windows_handle
file_descriptor.overlapped_operation(handle, "ReadFile", file_descriptor.read_timeout) do |overlapped|
ret = LibC.ReadFile(handle, slice, slice.size, out byte_count, overlapped)
{ret, byte_count}
end.to_i32
end

def write(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
handle = file_descriptor.windows_handle

file_descriptor.overlapped_operation(handle, "WriteFile", file_descriptor.write_timeout, writing: true) do |overlapped|
ret = LibC.WriteFile(handle, slice, slice.size, out byte_count, overlapped)
{ret, byte_count}
end.to_i32
end

def close(file_descriptor : Crystal::System::FileDescriptor) : Nil
LibC.CancelIoEx(file_descriptor.windows_handle, nil) unless file_descriptor.system_blocking?
end
end

class Crystal::Iocp::Event
Expand Down
15 changes: 5 additions & 10 deletions src/crystal/system/win32/file_descriptor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,7 @@ module Crystal::System::FileDescriptor
elsif system_blocking?
read_blocking(handle, slice)
else
overlapped_operation(handle, "ReadFile", read_timeout) do |overlapped|
ret = LibC.ReadFile(handle, slice, slice.size, out byte_count, overlapped)
{ret, byte_count}
end.to_i32
event_loop.read(self, slice)
end
end

Expand All @@ -53,10 +50,7 @@ module Crystal::System::FileDescriptor
if system_blocking?
write_blocking(handle, slice).to_i32
else
overlapped_operation(handle, "WriteFile", write_timeout, writing: true) do |overlapped|
ret = LibC.WriteFile(handle, slice, slice.size, out byte_count, overlapped)
{ret, byte_count}
end.to_i32
event_loop.write(self, slice)
end
end

Expand All @@ -75,7 +69,8 @@ module Crystal::System::FileDescriptor
bytes_written
end

private def system_blocking?
# :nodoc:
def system_blocking?
@system_blocking
end
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -167,7 +162,7 @@ module Crystal::System::FileDescriptor
end

private def system_close
LibC.CancelIoEx(windows_handle, nil) unless system_blocking?
event_loop.close(self)

file_descriptor_close
end
Expand Down
Loading