Let the event loop decide the blocking mode of File

spec/std/file_spec.cr

@@ -1,5 +1,6 @@
 require "./spec_helper"
 require "../support/thread"
+require "wait_group"
 
 private def it_raises_on_null_byte(operation, file = __FILE__, line = __LINE__, end_line = __END_LINE__, &block)
   it "errors on #{operation}", file, line, end_line do
@@ -47,112 +48,72 @@ describe "File" do
     end
   end
 
-  describe "blocking" do
-    it "opens regular file as blocking" do
-      with_tempfile("regular") do |path|
-        File.open(path, "w") do |file|
-          file.blocking.should be_true
-        end
+  {% if LibC.has_method?(:mkfifo) && !flag?(:darwin) %}
+    # interpreter doesn't support threads yet (#14287)
+    pending_interpreted "can read/write fifo file without blocking" do
+      path = File.tempname("chardev")
+      ret = LibC.mkfifo(path, File::DEFAULT_CREATE_PERMISSIONS)
+      raise RuntimeError.from_errno("mkfifo") unless ret == 0
 
-        File.open(path, "w", blocking: nil) do |file|
-          file.blocking.should be_true
-        end
+      # FIXME: open(2) will block when opening a fifo file until another thread
+      #        or process also opened the file
+      writer = nil
+      thread = new_thread do
+        writer = File.new(path, "w")
       end
-    end
 
-    it "opens regular file as non-blocking" do
-      with_tempfile("regular") do |path|
-        File.open(path, "w", blocking: false) do |file|
-          file.blocking.should be_false
-        end
-      end
-    end
+      rbuf = Bytes.new(5120)
+      wbuf = Bytes.new(5120)
+      Random::DEFAULT.random_bytes(wbuf)
 
-    {% if flag?(:unix) %}
-      if File.exists?("/dev/tty")
-        it "opens character device" do
-          File.open("/dev/tty", "r") do |file|
-            file.blocking.should be_true
-          end
+      File.open(path, "r") do |reader|
+        # opened fifo for read: wait for thread to open for write
+        thread.join
 
-          File.open("/dev/tty", "r", blocking: false) do |file|
-            file.blocking.should be_false
-          end
+        reader.read_timeout = 1.second
+        writer.not_nil!.write_timeout = 1.second
 
-          File.open("/dev/tty", "r", blocking: nil) do |file|
-            file.blocking.should be_false
+        WaitGroup.wait do |wg|
+          wg.spawn do
+            64.times do |i|
+              reader.read_fully(rbuf)
+            end
           end
-        rescue File::Error
-          # The TTY may not be available (e.g. Docker CI)
-        end
-      end
-
-      {% if LibC.has_method?(:mkfifo) %}
-        # interpreter doesn't support threads yet (#14287)
-        pending_interpreted "opens fifo file as non-blocking" do
-          path = File.tempname("chardev")
-          ret = LibC.mkfifo(path, File::DEFAULT_CREATE_PERMISSIONS)
-          raise RuntimeError.from_errno("mkfifo") unless ret == 0
 
-          # FIXME: open(2) will block when opening a fifo file until another
-          #        thread or process also opened the file; we should pass
-          #        O_NONBLOCK to the open(2) call itself, not afterwards
-          file = nil
-          new_thread { file = File.new(path, "w", blocking: nil) }
-
-          begin
-            File.open(path, "r", blocking: false) do |file|
-              file.blocking.should be_false
+          wg.spawn do
+            64.times do |i|
+              writer.not_nil!.write(wbuf)
             end
-          ensure
-            File.delete(path)
-            file.try(&.close)
+            writer.not_nil!.close
           end
         end
-      {% end %}
-    {% end %}
-
-    it "reads non-blocking file" do
-      File.open(datapath("test_file.txt"), "r", blocking: false) do |f|
-        f.gets_to_end.should eq("Hello World\n" * 20)
-      end
-    end
-
-    it "writes and reads large non-blocking file" do
-      with_tempfile("non-blocking-io.txt") do |path|
-        File.open(path, "w+", blocking: false) do |f|
-          f.puts "Hello World\n" * 40000
-          f.pos = 0
-          f.gets_to_end.should eq("Hello World\n" * 40000)
-        end
       end
-    end
 
-    it "can append non-blocking to an existing file" do
-      with_tempfile("append-existing.txt") do |path|
-        File.write(path, "hello")
-        File.write(path, " world", mode: "a", blocking: false)
-        File.read(path).should eq("hello world")
-      end
+      rbuf.should eq(wbuf)
+    ensure
+      File.delete(path) if path
+      writer.try(&.close)
     end
+  {% end %}
 
-    it "returns the actual position after non-blocking append" do
-      with_tempfile("delete-file.txt") do |filename|
-        File.write(filename, "hello")
-
-        File.open(filename, "a", blocking: false) do |file|
-          file.tell.should eq(0)
+  # This test verifies that the workaround for a win32 bug with the O_APPEND
+  # equivalent with OVERLAPPED operations is working as expected.
+  it "returns the actual position after append" do
+    with_tempfile("delete-file.txt") do |filename|
+      File.write(filename, "hello")
 
-          file.write "12345".to_slice
-          file.tell.should eq(10)
+      File.open(filename, "a") do |file|
+        file.tell.should eq(0)
 
-          file.seek(5, IO::Seek::Set)
-          file.write "6789".to_slice
-          file.tell.should eq(14)
-        end
+        file.write "12345".to_slice
+        file.tell.should eq(10)
 
-        File.read(filename).should eq("hello123456789")
+        file.seek(5, IO::Seek::Set)
+        file.write "6789".to_slice
+        file.tell.should eq(14)
       end
+
+      File.read(filename).should eq("hello123456789")
     end
   end
 

src/crystal/event_loop/iocp.cr

@@ -244,7 +244,7 @@ class Crystal::EventLoop::IOCP < Crystal::EventLoop
   end
 
   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)
+    access, disposition, attributes = System::File.posix_to_open_opts(flags, permissions, !!blocking)
 
     handle = LibC.CreateFileW(
       System.to_wstr(path),

src/crystal/event_loop/libevent.cr

@@ -130,13 +130,14 @@ class Crystal::EventLoop::LibEvent < Crystal::EventLoop
     fd = LibC.open(path, flags | LibC::O_CLOEXEC, permissions)
     return Errno.value if fd == -1
 
-    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}
+    {% if flag?(:darwin) %}
+      # FIXME: poll of non-blocking fifo fd on darwin appears to be broken, so
+      # we default to blocking for the time being
+      blocking = true if blocking.nil?
+    {% end %}
+
+    System::FileDescriptor.set_blocking(fd, false) unless blocking
+    {fd, !!blocking}
   end
 
   def read(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32

src/crystal/event_loop/polling.cr

@@ -176,13 +176,14 @@ abstract class Crystal::EventLoop::Polling < Crystal::EventLoop
     fd = LibC.open(path, flags | LibC::O_CLOEXEC, permissions)
     return Errno.value if fd == -1
 
-    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}
+    {% if flag?(:darwin) %}
+      # FIXME: poll of non-blocking fifo fd on darwin appears to be broken, so
+      # we default to blocking for the time being
+      blocking = true if blocking.nil?
+    {% end %}
+
+    System::FileDescriptor.set_blocking(fd, false) unless blocking
+    {fd, !!blocking}
   end
 
   def read(file_descriptor : System::FileDescriptor, slice : Bytes) : Int32

src/crystal/system/unix/file_descriptor.cr

@@ -24,14 +24,18 @@ module Crystal::System::FileDescriptor
   end
 
   private def system_blocking=(value)
-    current_flags = fcntl(LibC::F_GETFL)
+    FileDescriptor.set_blocking(fd, value)
+  end
+
+  protected def self.set_blocking(fd, value)
+    current_flags = fcntl(fd, LibC::F_GETFL)
     new_flags = current_flags
     if value
       new_flags &= ~LibC::O_NONBLOCK
     else
       new_flags |= LibC::O_NONBLOCK
     end
-    fcntl(LibC::F_SETFL, new_flags) unless new_flags == current_flags
+    fcntl(fd, LibC::F_SETFL, new_flags) unless new_flags == current_flags
   end
 
   protected def system_blocking_init(blocking : Bool?)

src/file.cr

@@ -128,7 +128,7 @@ class File < IO::FileDescriptor
   # 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)
+  private def initialize(@path, fd : Int, mode = "", blocking = nil, encoding = nil, invalid = nil)
     super(handle: fd)
     system_init(mode, blocking)
     set_encoding(encoding, invalid: invalid) if encoding
@@ -156,19 +156,16 @@ class File < IO::FileDescriptor
   # Line endings are preserved on all platforms. The `b` mode flag has no
   # effect; it is provided only for POSIX compatibility.
   #
-  # *blocking* is set to `true` by default because system event queues (e.g.
-  # epoll, kqueue) will always report the file descriptor of regular disk files
-  # as ready.
+  # NOTE: The *blocking* arg is deprecated since Crystal 1.17. It used to be
+  # true by default to denote a regular disk file (always ready in system event
+  # loops) and could be set to false when the file was known to be a fifo, pipe,
+  # or character device (for example `/dev/tty`). The event loop now chooses
+  # the appropriate blocking mode automatically and there are no reasons to
+  # change it anymore.
   #
-  # *blocking* must be set to `false` on POSIX targets when the file to open
-  # isn't a regular file but a character device (e.g. `/dev/tty`) or fifo. These
-  # files depend on another process or thread to also be reading or writing, and
-  # system event queues will properly report readiness.
-  #
-  # *blocking* may also be set to `nil` in which case the blocking or
-  # non-blocking flag will be determined automatically, at the expense of an
-  # additional syscall.
-  def self.new(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = true)
+  # NOTE: On macOS files are always opened in blocking mode because non-blocking
+  # FIFO files don't work — the OS exhibits issues with readiness notifications.
+  def self.new(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = nil)
     filename = filename.to_s
     fd, blocking = Crystal::System::File.open(filename, mode, perm: perm, blocking: blocking)
     new(filename, fd, mode, blocking, encoding, invalid)
@@ -505,7 +502,7 @@ class File < IO::FileDescriptor
   # permissions may be set using the *perm* parameter.
   #
   # See `self.new` for what *mode* can be.
-  def self.open(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = true) : self
+  def self.open(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = nil) : self
     new filename, mode, perm, encoding, invalid, blocking
   end
 
@@ -514,7 +511,7 @@ class File < IO::FileDescriptor
   # file as an argument, the file will be automatically closed when the block returns.
   #
   # See `self.new` for what *mode* can be.
-  def self.open(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = true, &)
+  def self.open(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = nil, &)
     file = new filename, mode, perm, encoding, invalid, blocking
     begin
       yield file
@@ -529,7 +526,7 @@ class File < IO::FileDescriptor
   # File.write("bar", "foo")
   # File.read("bar") # => "foo"
   # ```
-  def self.read(filename : Path | String, encoding = nil, invalid = nil, blocking = true) : String
+  def self.read(filename : Path | String, encoding = nil, invalid = nil, blocking = nil) : String
     open(filename, "r", blocking: blocking) do |file|
       if encoding
         file.set_encoding(encoding, invalid: invalid)
@@ -558,7 +555,7 @@ class File < IO::FileDescriptor
   # end
   # array # => ["foo", "bar"]
   # ```
-  def self.each_line(filename : Path | String, encoding = nil, invalid = nil, chomp = true, blocking = true, &)
+  def self.each_line(filename : Path | String, encoding = nil, invalid = nil, chomp = true, blocking = nil, &)
     open(filename, "r", encoding: encoding, invalid: invalid, blocking: blocking) do |file|
       file.each_line(chomp: chomp) do |line|
         yield line
@@ -572,7 +569,7 @@ class File < IO::FileDescriptor
   # File.write("foobar", "foo\nbar")
   # File.read_lines("foobar") # => ["foo", "bar"]
   # ```
-  def self.read_lines(filename : Path | String, encoding = nil, invalid = nil, chomp = true, blocking = true) : Array(String)
+  def self.read_lines(filename : Path | String, encoding = nil, invalid = nil, chomp = true, blocking = nil) : Array(String)
     lines = [] of String
     each_line(filename, encoding: encoding, invalid: invalid, chomp: chomp, blocking: blocking) do |line|
       lines << line
@@ -597,7 +594,7 @@ class File < IO::FileDescriptor
   # (the result of invoking `to_s` on *content*).
   #
   # See `self.new` for what *mode* can be.
-  def self.write(filename : Path | String, content, perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, mode = "w", blocking = true)
+  def self.write(filename : Path | String, content, perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, mode = "w", blocking = nil)
     open(filename, mode, perm, encoding: encoding, invalid: invalid, blocking: blocking) do |file|
       case content
       when Bytes