Crystal::EventLoop::FileDescriptor#open now sets the non/blocking flag (#15754)

ysbaddaden · web-flow · commit 3d1ffcc8503e · 2025-05-19T10:52:37.000+02:00
Moves the responsibility to set `FILE_FLAG_OVERLAPPED` or `O_NONBLOCK` to each event loop, so they can eventually own the blocking flag.

There is no change in behavior, yet: the event loops still set the flag depending on the `blocking` argument.

Refactors the `IO::FileDescriptor` and `File` internal constructors so we can instantiate the object without changing the blocking state (already set).
diff --git a/spec/std/file/tempfile_spec.cr b/spec/std/file/tempfile_spec.cr
@@ -197,7 +197,7 @@ describe Crystal::System::File do
     it "creates random file name" do
       with_tempfile "random-path" do |tempdir|
         Dir.mkdir tempdir
-        fd, path = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14]))
+        fd, path, _ = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14]))
         path.should eq Path[tempdir, "A789abcdeZ"].to_s
       ensure
         IO::FileDescriptor.new(fd).close if fd
@@ -209,7 +209,7 @@ describe Crystal::System::File do
         Dir.mkdir tempdir
         existing_path = Path[tempdir, "A789abcdeZ"]
         File.touch existing_path
-        fd, path = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22]))
+        fd, path, _ = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22]))
         path.should eq File.join(tempdir, "AfghijklmZ")
       ensure
         IO::FileDescriptor.new(fd).close if fd
@@ -221,7 +221,7 @@ describe Crystal::System::File do
         Dir.mkdir tempdir
         File.touch Path[tempdir, "A789abcdeZ"]
         expect_raises(File::AlreadyExistsError, "Error creating temporary file") do
-          fd, path = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14]))
+          fd, path, _ = Crystal::System::File.mktemp("A", "Z", dir: tempdir, random: TestRNG.new([7, 8, 9, 10, 11, 12, 13, 14]))
         ensure
           IO::FileDescriptor.new(fd).close if fd
         end
diff --git a/src/crystal/event_loop/file_descriptor.cr b/src/crystal/event_loop/file_descriptor.cr
@@ -7,7 +7,7 @@ abstract class Crystal::EventLoop
     # `nil`.
     #
     # Returns the system file descriptor or handle, or a system error.
-    abstract def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : System::FileDescriptor::Handle | Errno | WinError
+    abstract def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : {System::FileDescriptor::Handle, Bool} | Errno | WinError
 
     # Reads at least one byte from the file descriptor into *slice*.
     #
diff --git a/src/crystal/event_loop/iocp.cr b/src/crystal/event_loop/iocp.cr
@@ -223,7 +223,7 @@ class Crystal::EventLoop::IOCP < Crystal::EventLoop
     FiberEvent.new(:select_timeout, fiber)
   end
 
-  def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : System::FileDescriptor::Handle | WinError
+  def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : {System::FileDescriptor::Handle, Bool} | WinError
     access, disposition, attributes = System::File.posix_to_open_opts(flags, permissions, blocking)
 
     handle = LibC.CreateFileW(
@@ -239,7 +239,8 @@ class Crystal::EventLoop::IOCP < Crystal::EventLoop
     if handle == LibC::INVALID_HANDLE_VALUE
       WinError.value
     else
-      handle.address
+      create_completion_port(handle) unless blocking
+      {handle.address, !!blocking}
     end
   end
 
diff --git a/src/crystal/event_loop/libevent.cr b/src/crystal/event_loop/libevent.cr
@@ -108,16 +108,19 @@ class Crystal::EventLoop::LibEvent < Crystal::EventLoop
     end
   end
 
-  def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : System::FileDescriptor::Handle | Errno
+  def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : {System::FileDescriptor::Handle, Bool} | Errno
     path.check_no_null_byte
 
     fd = LibC.open(path, flags | LibC::O_CLOEXEC, permissions)
+    return Errno.value if fd == -1
 
-    if fd == -1
-      Errno.value
-    else
-      fd
+    blocking = !System::File.special_type?(fd) if blocking.nil?
+    unless blocking
+      status_flags = System::FileDescriptor.fcntl(fd, LibC::F_GETFL)
+      System::FileDescriptor.fcntl(fd, LibC::F_SETFL, status_flags | LibC::O_NONBLOCK)
     end
+
+    {fd, blocking}
   end
 
   def read(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32
diff --git a/src/crystal/event_loop/polling.cr b/src/crystal/event_loop/polling.cr
@@ -154,16 +154,19 @@ abstract class Crystal::EventLoop::Polling < Crystal::EventLoop
 
   # file descriptor interface, see Crystal::EventLoop::FileDescriptor
 
-  def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : System::FileDescriptor::Handle | Errno
+  def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : {System::FileDescriptor::Handle, Bool} | Errno
     path.check_no_null_byte
 
     fd = LibC.open(path, flags | LibC::O_CLOEXEC, permissions)
+    return Errno.value if fd == -1
 
-    if fd == -1
-      Errno.value
-    else
-      fd
+    blocking = !System::File.special_type?(fd) if blocking.nil?
+    unless blocking
+      status_flags = System::FileDescriptor.fcntl(fd, LibC::F_GETFL)
+      System::FileDescriptor.fcntl(fd, LibC::F_SETFL, status_flags | LibC::O_NONBLOCK)
     end
+
+    {fd, blocking}
   end
 
   def read(file_descriptor : System::FileDescriptor, slice : Bytes) : Int32
diff --git a/src/crystal/event_loop/wasi.cr b/src/crystal/event_loop/wasi.cr
@@ -28,7 +28,7 @@ class Crystal::EventLoop::Wasi < Crystal::EventLoop
     raise NotImplementedError.new("Crystal::Wasi::EventLoop.create_fd_read_event")
   end
 
-  def open(filename : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : System::FileDescriptor::Handle | Errno | WinError
+  def open(filename : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : {System::FileDescriptor::Handle, Bool} | Errno | WinError
     raise NotImplementedError.new("Crystal::Wasi::EventLoop#open")
   end
 
diff --git a/src/crystal/system/file.cr b/src/crystal/system/file.cr
@@ -51,7 +51,7 @@ module Crystal::System::File
 
   LOWER_ALPHANUM = "0123456789abcdefghijklmnopqrstuvwxyz".to_slice
 
-  def self.mktemp(prefix : String?, suffix : String?, dir : String, random : ::Random = ::Random::DEFAULT) : {FileDescriptor::Handle, String}
+  def self.mktemp(prefix : String?, suffix : String?, dir : String, random : ::Random = ::Random::DEFAULT) : {FileDescriptor::Handle, String, Bool}
     flags = LibC::O_RDWR | LibC::O_CREAT | LibC::O_EXCL
     perm = ::File::Permissions.new(0o600)
 
@@ -68,8 +68,9 @@ module Crystal::System::File
       end
 
       case result = EventLoop.current.open(path, flags, perm, blocking: true)
-      when FileDescriptor::Handle
-        return {result, path}
+      when Tuple(FileDescriptor::Handle, Bool)
+        fd, blocking = result
+        return {fd, path, blocking}
       when Errno::EEXIST, WinError::ERROR_FILE_EXISTS
         # retry
       else
diff --git a/src/crystal/system/unix/file.cr b/src/crystal/system/unix/file.cr
@@ -3,18 +3,25 @@ require "file/error"
 
 # :nodoc:
 module Crystal::System::File
-  def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions, blocking : Bool?) : FileDescriptor::Handle
+  def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions, blocking : Bool?) : {FileDescriptor::Handle, Bool}
     perm = ::File::Permissions.new(perm) if perm.is_a? Int32
 
     case result = EventLoop.current.open(filename, open_flag(mode), perm, blocking)
-    in FileDescriptor::Handle
+    in Tuple(FileDescriptor::Handle, Bool)
       result
     in Errno
       raise ::File::Error.from_os_error("Error opening file with mode '#{mode}'", result, file: filename)
     end
   end
 
-  protected def system_set_mode(mode : String)
+  protected def system_init(mode : String, blocking : Bool) : Nil
+  end
+
+  def self.special_type?(fd)
+    stat = uninitialized LibC::Stat
+    ret = fstat(fd, pointerof(stat))
+    # not checking for S_IFSOCK because we can't open(2) a socket
+    ret != -1 && (stat.st_mode & LibC::S_IFMT).in?(LibC::S_IFCHR, LibC::S_IFIFO)
   end
 
   def self.info?(path : String, follow_symlinks : Bool) : ::File::Info?
diff --git a/src/crystal/system/unix/file_descriptor.cr b/src/crystal/system/unix/file_descriptor.cr
@@ -34,7 +34,7 @@ module Crystal::System::FileDescriptor
     fcntl(LibC::F_SETFL, new_flags) unless new_flags == current_flags
   end
 
-  private def system_blocking_init(blocking : Bool?)
+  protected def system_blocking_init(blocking : Bool?)
     if blocking.nil?
       blocking =
         case system_info.type
diff --git a/src/crystal/system/wasi/file.cr b/src/crystal/system/wasi/file.cr
@@ -2,7 +2,7 @@ require "../unix/file"
 
 # :nodoc:
 module Crystal::System::File
-  protected def system_set_mode(mode : String)
+  protected def system_init(mode : String, blocking : Bool) : Nil
   end
 
   def self.chmod(path, mode)
diff --git a/src/crystal/system/wasi/file_descriptor.cr b/src/crystal/system/wasi/file_descriptor.cr
@@ -17,7 +17,7 @@ module Crystal::System::FileDescriptor
     r
   end
 
-  private def system_blocking_init(blocking : Bool?)
+  protected def system_blocking_init(blocking : Bool?)
   end
 
   private def system_reopen(other : IO::FileDescriptor)
diff --git a/src/crystal/system/win32/file.cr b/src/crystal/system/win32/file.cr
@@ -14,7 +14,7 @@ module Crystal::System::File
   # write at the end of the file.
   getter? system_append = false
 
-  def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions, blocking : Bool?) : FileDescriptor::Handle
+  def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions, blocking : Bool?) : {FileDescriptor::Handle, Bool}
     perm = ::File::Permissions.new(perm) if perm.is_a? Int32
     # Only the owner writable bit is used, since windows only supports
     # the read only attribute.
@@ -25,7 +25,7 @@ module Crystal::System::File
     end
 
     case result = EventLoop.current.open(filename, open_flag(mode), ::File::Permissions.new(perm), blocking != false)
-    in FileDescriptor::Handle
+    in Tuple(FileDescriptor::Handle, Bool)
       result
     in WinError
       raise ::File::Error.from_os_error("Error opening file with mode '#{mode}'", result, file: filename)
@@ -88,8 +88,9 @@ module Crystal::System::File
     {access, disposition, attributes}
   end
 
-  protected def system_set_mode(mode : String)
+  protected def system_init(mode : String, blocking : Bool) : Nil
     @system_append = true if mode.starts_with?('a')
+    @system_blocking = blocking
   end
 
   private def write_blocking(handle, slice)
diff --git a/src/crystal/system/win32/file_descriptor.cr b/src/crystal/system/win32/file_descriptor.cr
@@ -104,7 +104,7 @@ module Crystal::System::FileDescriptor
     end
   end
 
-  def system_blocking_init(blocking : Bool?)
+  protected def system_blocking_init(blocking : Bool?)
     if blocking.nil?
       # there are no official API to know whether a handle has been opened with
       # the OVERLAPPED flag, but the following call is supposed to leak the
diff --git a/src/file.cr b/src/file.cr
@@ -125,16 +125,13 @@ class File < IO::FileDescriptor
 
   include Crystal::System::File
 
-  # This constructor is provided for subclasses to be able to initialize an
-  # `IO::FileDescriptor` with a *path* and *fd*.
-  private def initialize(@path, fd : Int, blocking = false, encoding = nil, invalid = nil)
-    self.set_encoding(encoding, invalid: invalid) if encoding
-    super(fd, blocking)
-  end
-
-  # :nodoc:
-  def self.from_fd(path : String, fd : Int, *, blocking = false, encoding = nil, invalid = nil)
-    new(path, fd, blocking: blocking, encoding: encoding, invalid: invalid)
+  # This constructor is for constructors to be able to initialize a `File` with
+  # a *path* and *fd*. The *blocking* param is informational and must reflect
+  # the non/blocking state of the underlying fd.
+  private def initialize(@path, fd : Int, mode = "", blocking = true, encoding = nil, invalid = nil)
+    super(handle: fd)
+    system_init(mode, blocking)
+    set_encoding(encoding, invalid: invalid) if encoding
   end
 
   # Opens the file named by *filename*.
@@ -173,8 +170,8 @@ class File < IO::FileDescriptor
   # additional syscall.
   def self.new(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = true)
     filename = filename.to_s
-    fd = Crystal::System::File.open(filename, mode, perm: perm, blocking: blocking)
-    new(filename, fd, blocking: blocking, encoding: encoding, invalid: invalid).tap { |f| f.system_set_mode(mode) }
+    fd, blocking = Crystal::System::File.open(filename, mode, perm: perm, blocking: blocking)
+    new(filename, fd, mode, blocking, encoding, invalid)
   end
 
   getter path : String
diff --git a/src/file/tempfile.cr b/src/file/tempfile.cr
@@ -64,8 +64,8 @@ class File
   #
   # It is the caller's responsibility to remove the file when no longer needed.
   def self.tempfile(prefix : String?, suffix : String?, *, dir : String = Dir.tempdir, encoding = nil, invalid = nil)
-    fileno, path = Crystal::System::File.mktemp(prefix, suffix, dir)
-    new(path, fileno, blocking: true, encoding: encoding, invalid: invalid)
+    fileno, path, blocking = Crystal::System::File.mktemp(prefix, suffix, dir)
+    new(path, fileno, blocking: blocking, encoding: encoding, invalid: invalid)
   end
 
   # Creates a temporary file.
diff --git a/src/io/file_descriptor.cr b/src/io/file_descriptor.cr
@@ -39,11 +39,17 @@ class IO::FileDescriptor < IO
     write_timeout
   end
 
-  def initialize(fd : Handle, blocking = nil, *, @close_on_finalize = true)
-    @volatile_fd = Atomic.new(fd)
+  def self.new(fd : Handle, blocking = nil, *, close_on_finalize = true)
+    file_descriptor = new(handle: fd, close_on_finalize: close_on_finalize)
+    file_descriptor.system_blocking_init(blocking) unless file_descriptor.closed?
+    file_descriptor
+  end
+
+  # :nodoc:
+  def initialize(*, handle : Handle, @close_on_finalize = true)
+    @volatile_fd = Atomic.new(handle)
     @closed = true # This is necessary so we can reference `self` in `system_closed?` (in case of an exception)
     @closed = system_closed?
-    system_blocking_init(blocking) unless @closed
   end
 
   # :nodoc:

Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@ abstract class Crystal::EventLoop`
`7`	`7`	# `nil`.
`8`	`8`	`#`
`9`	`9`	`# Returns the system file descriptor or handle, or a system error.`
`10`		`- abstract def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : System::FileDescriptor::Handle \| Errno \| WinError`
	`10`	`+ abstract def open(path : String, flags : Int32, permissions : File::Permissions, blocking : Bool?) : {System::FileDescriptor::Handle, Bool} \| Errno \| WinError`
`11`	`11`
`12`	`12`	`# Reads at least one byte from the file descriptor into slice.`
`13`	`13`	`#`