diff --git a/spec/std/file_spec.cr b/spec/std/file_spec.cr index 9b3b7bfce6d3..e4ef82b6f648 100644 --- a/spec/std/file_spec.cr +++ b/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 diff --git a/src/crystal/event_loop/iocp.cr b/src/crystal/event_loop/iocp.cr index 8f06889d98c5..a5f98ed289bd 100644 --- a/src/crystal/event_loop/iocp.cr +++ b/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), diff --git a/src/crystal/event_loop/libevent.cr b/src/crystal/event_loop/libevent.cr index 271732fa5fda..82190d153cde 100644 --- a/src/crystal/event_loop/libevent.cr +++ b/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 diff --git a/src/crystal/event_loop/polling.cr b/src/crystal/event_loop/polling.cr index b9226e60c268..5ad559820791 100644 --- a/src/crystal/event_loop/polling.cr +++ b/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 diff --git a/src/crystal/system/unix/file_descriptor.cr b/src/crystal/system/unix/file_descriptor.cr index 41e856623b59..f671fd5da842 100644 --- a/src/crystal/system/unix/file_descriptor.cr +++ b/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?) diff --git a/src/file.cr b/src/file.cr index 429eb8b3dab4..9bd2f120f597 100644 --- a/src/file.cr +++ b/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