From 44a1b5e8fc8daadf099d5af7ba7ae854bc752ea7 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Fri, 26 Jan 2024 14:59:12 +0100 Subject: [PATCH 1/7] Fix: opening/reading from fifo/chardev files are blocking the thread Disk files are always opened in a blocking way because epoll (for instance) will always say that a file is ready for reach or write. But this is only for regular disk files, and that doesn't apply to pipes and characters devices for example. Opening `/dev/tty` (a character device) will block the current thread when trying to read from it until you start typing on the keyboard. IO::FileDescriptor was taking care to check the file type, but File always told it to be blocking. Worse, trying to open a fifo/pipe file will block the current thread until another thread or process also opens it, which means that we must determine whether to block _before_ trying to open a file (not change it afterwards). I also added a `blocking` parameter to `File.open` and `File.new`. --- spec/std/file_spec.cr | 26 ++++++++++++++++++++++++++ src/crystal/system/unix/file.cr | 7 ++++--- src/crystal/system/win32/file.cr | 6 +++--- src/file.cr | 25 +++++++++++++++++-------- 4 files changed, 50 insertions(+), 14 deletions(-) diff --git a/spec/std/file_spec.cr b/spec/std/file_spec.cr index 3c020fee36a1..e2293a7ecf2f 100644 --- a/spec/std/file_spec.cr +++ b/spec/std/file_spec.cr @@ -47,6 +47,32 @@ 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 + end + end + + {% if LibC.has_method?(:mkfifo) %} + it "opens character device/pipe 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 + + begin + File.open(path, "r") do |file| + file.blocking.should be_false + end + ensure + File.delete(path) + end + end + {% end %} + end + it "reads entire file" do str = File.read datapath("test_file.txt") str.should eq("Hello World\n" * 20) diff --git a/src/crystal/system/unix/file.cr b/src/crystal/system/unix/file.cr index ca04693d0b1a..3d1210c5e3e0 100644 --- a/src/crystal/system/unix/file.cr +++ b/src/crystal/system/unix/file.cr @@ -3,10 +3,10 @@ require "file/error" # :nodoc: module Crystal::System::File - def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions) + def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions, blocking : Bool?) perm = ::File::Permissions.new(perm) if perm.is_a? Int32 - fd, errno = open(filename, open_flag(mode), perm) + fd, errno = open(filename, open_flag(mode), perm, blocking) unless errno.none? raise ::File::Error.from_os_error("Error opening file with mode '#{mode}'", errno, file: filename) @@ -15,9 +15,10 @@ module Crystal::System::File fd end - def self.open(filename : String, flags : Int32, perm : ::File::Permissions) : {LibC::Int, Errno} + def self.open(filename : String, flags : Int32, perm : ::File::Permissions, blocking : Bool?) : {LibC::Int, Errno} filename.check_no_null_byte flags |= LibC::O_CLOEXEC + flags |= LibC::O_NONBLOCK if blocking == false fd = LibC.open(filename, flags, perm) diff --git a/src/crystal/system/win32/file.cr b/src/crystal/system/win32/file.cr index 43b2dd8142ec..cc079faba9c9 100644 --- a/src/crystal/system/win32/file.cr +++ b/src/crystal/system/win32/file.cr @@ -9,7 +9,7 @@ require "c/ntifs" require "c/winioctl" module Crystal::System::File - def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions) : LibC::Int + def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions, blocking : Bool?) : LibC::Int 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. @@ -19,7 +19,7 @@ module Crystal::System::File perm = LibC::S_IREAD end - fd, errno = open(filename, open_flag(mode), ::File::Permissions.new(perm)) + fd, errno = open(filename, open_flag(mode), ::File::Permissions.new(perm), blocking) unless errno.none? raise ::File::Error.from_os_error("Error opening file with mode '#{mode}'", errno, file: filename) end @@ -27,7 +27,7 @@ module Crystal::System::File fd end - def self.open(filename : String, flags : Int32, perm : ::File::Permissions) : {LibC::Int, Errno} + def self.open(filename : String, flags : Int32, perm : ::File::Permissions, blocking : Bool?) : {LibC::Int, Errno} access, disposition, attributes = self.posix_to_open_opts flags, perm handle = LibC.CreateFileW( diff --git a/src/file.cr b/src/file.cr index c88beadb0de4..4446364f153f 100644 --- a/src/file.cr +++ b/src/file.cr @@ -126,7 +126,7 @@ class File < IO::FileDescriptor # This constructor is provided for subclasses to be able to initialize an # `IO::FileDescriptor` with a *path* and *fd*. - private def initialize(@path, fd, blocking = false, encoding = nil, invalid = nil) + private def initialize(@path, fd : Int, blocking = false, encoding = nil, invalid = nil) self.set_encoding(encoding, invalid: invalid) if encoding super(fd, blocking) end @@ -157,10 +157,19 @@ 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. - def self.new(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil) + def self.new(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = nil) filename = filename.to_s - fd = Crystal::System::File.open(filename, mode, perm) - new(filename, fd, blocking: true, encoding: encoding, invalid: invalid) + + if blocking.nil? + if type = File.info?(filename).try(&.type) + blocking = !(type.pipe? || type.socket? || type.character_device?) + else + blocking = true + end + end + + fd = Crystal::System::File.open(filename, mode, perm: perm, blocking: blocking) + new(filename, fd, blocking: blocking, encoding: encoding, invalid: invalid) end getter path : String @@ -712,8 +721,8 @@ 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) : self - new filename, mode, perm, encoding, invalid + 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 # Opens the file named by *filename*. If a file is being created, its initial @@ -721,8 +730,8 @@ 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, &) - file = new filename, mode, perm, encoding, invalid + 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 ensure From f209e08f24c61b07a4686c96808d4f0f4637f356 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Fri, 26 Jan 2024 15:28:12 +0100 Subject: [PATCH 2/7] fixup! Fix: opening/reading from fifo/chardev files are blocking the thread --- src/crystal/system/file.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crystal/system/file.cr b/src/crystal/system/file.cr index 89d29512c28e..1733acefdc08 100644 --- a/src/crystal/system/file.cr +++ b/src/crystal/system/file.cr @@ -65,7 +65,7 @@ module Crystal::System::File io << suffix end - fd, errno = open(path, mode, perm) + fd, errno = open(path, mode, perm, blocking: true) if errno.none? return {fd, path} From 46146a5050daf3fc4fdcc600395a14eee42a21a9 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 29 Jan 2024 11:10:19 +0100 Subject: [PATCH 3/7] Require explicit blocking parameter to File Removes the automatic detection of the file type and blocking behavior before opening the file as we must support at least EINTR and ENXIO return values for `open(2)`. Blocking still defaults to true. Developers may pass nil for auto detection or false to force the nonblocking flag (after open). Opening a fifo without another thread/process also trying to open for read or write will still block the current thread (will be another PR). --- spec/std/file_spec.cr | 47 ++++++++++++++++++++++++++------ src/crystal/system/unix/file.cr | 7 ++--- src/crystal/system/win32/file.cr | 6 ++-- src/file.cr | 46 +++++++++++++++++-------------- 4 files changed, 69 insertions(+), 37 deletions(-) diff --git a/spec/std/file_spec.cr b/spec/std/file_spec.cr index e2293a7ecf2f..5b2c848bfb7a 100644 --- a/spec/std/file_spec.cr +++ b/spec/std/file_spec.cr @@ -53,23 +53,52 @@ describe "File" do File.open(path, "w") do |file| file.blocking.should be_true end + + File.open(path, "w", blocking: nil) do |file| + file.blocking.should be_true + end end end - {% if LibC.has_method?(:mkfifo) %} - it "opens character device/pipe 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 + {% 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("/dev/tty", "r", blocking: false) do |file| + file.blocking.should be_false + end - begin - File.open(path, "r") do |file| + File.open("/dev/tty", "r", blocking: nil) do |file| file.blocking.should be_false end - ensure - File.delete(path) end end + + {% if LibC.has_method?(:mkfifo) %} + it "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 + Thread.new { file = File.new(path, "w", blocking: nil) } + + begin + File.open(path, "r", blocking: false) do |file| + file.blocking.should be_false + end + ensure + File.delete(path) + file.try(&.close) + end + end + {% end %} {% end %} end diff --git a/src/crystal/system/unix/file.cr b/src/crystal/system/unix/file.cr index 3d1210c5e3e0..ca04693d0b1a 100644 --- a/src/crystal/system/unix/file.cr +++ b/src/crystal/system/unix/file.cr @@ -3,10 +3,10 @@ require "file/error" # :nodoc: module Crystal::System::File - def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions, blocking : Bool?) + def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions) perm = ::File::Permissions.new(perm) if perm.is_a? Int32 - fd, errno = open(filename, open_flag(mode), perm, blocking) + fd, errno = open(filename, open_flag(mode), perm) unless errno.none? raise ::File::Error.from_os_error("Error opening file with mode '#{mode}'", errno, file: filename) @@ -15,10 +15,9 @@ module Crystal::System::File fd end - def self.open(filename : String, flags : Int32, perm : ::File::Permissions, blocking : Bool?) : {LibC::Int, Errno} + def self.open(filename : String, flags : Int32, perm : ::File::Permissions) : {LibC::Int, Errno} filename.check_no_null_byte flags |= LibC::O_CLOEXEC - flags |= LibC::O_NONBLOCK if blocking == false fd = LibC.open(filename, flags, perm) diff --git a/src/crystal/system/win32/file.cr b/src/crystal/system/win32/file.cr index cc079faba9c9..43b2dd8142ec 100644 --- a/src/crystal/system/win32/file.cr +++ b/src/crystal/system/win32/file.cr @@ -9,7 +9,7 @@ require "c/ntifs" require "c/winioctl" module Crystal::System::File - def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions, blocking : Bool?) : LibC::Int + def self.open(filename : String, mode : String, perm : Int32 | ::File::Permissions) : LibC::Int 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. @@ -19,7 +19,7 @@ module Crystal::System::File perm = LibC::S_IREAD end - fd, errno = open(filename, open_flag(mode), ::File::Permissions.new(perm), blocking) + fd, errno = open(filename, open_flag(mode), ::File::Permissions.new(perm)) unless errno.none? raise ::File::Error.from_os_error("Error opening file with mode '#{mode}'", errno, file: filename) end @@ -27,7 +27,7 @@ module Crystal::System::File fd end - def self.open(filename : String, flags : Int32, perm : ::File::Permissions, blocking : Bool?) : {LibC::Int, Errno} + def self.open(filename : String, flags : Int32, perm : ::File::Permissions) : {LibC::Int, Errno} access, disposition, attributes = self.posix_to_open_opts flags, perm handle = LibC.CreateFileW( diff --git a/src/file.cr b/src/file.cr index 4446364f153f..a011979dcc9b 100644 --- a/src/file.cr +++ b/src/file.cr @@ -157,18 +157,22 @@ 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. - def self.new(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = nil) + # + # *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. + # + # *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 readyness. + # + # *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) filename = filename.to_s - - if blocking.nil? - if type = File.info?(filename).try(&.type) - blocking = !(type.pipe? || type.socket? || type.character_device?) - else - blocking = true - end - end - - fd = Crystal::System::File.open(filename, mode, perm: perm, blocking: blocking) + fd = Crystal::System::File.open(filename, mode, perm: perm) new(filename, fd, blocking: blocking, encoding: encoding, invalid: invalid) end @@ -721,7 +725,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 = nil) : self + def self.open(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = true) : self new filename, mode, perm, encoding, invalid, blocking end @@ -730,7 +734,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 = nil, &) + def self.open(filename : Path | String, mode = "r", perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, blocking = true, &) file = new filename, mode, perm, encoding, invalid, blocking begin yield file @@ -745,8 +749,8 @@ class File < IO::FileDescriptor # File.write("bar", "foo") # File.read("bar") # => "foo" # ``` - def self.read(filename : Path | String, encoding = nil, invalid = nil) : String - open(filename, "r") do |file| + def self.read(filename : Path | String, encoding = nil, invalid = nil, blocking = true) : String + open(filename, "r", blocking: blocking) do |file| if encoding file.set_encoding(encoding, invalid: invalid) file.gets_to_end @@ -774,8 +778,8 @@ class File < IO::FileDescriptor # end # array # => ["foo", "bar"] # ``` - def self.each_line(filename : Path | String, encoding = nil, invalid = nil, chomp = true, &) - open(filename, "r", encoding: encoding, invalid: invalid) do |file| + def self.each_line(filename : Path | String, encoding = nil, invalid = nil, chomp = true, blocking = true, &) + open(filename, "r", encoding: encoding, invalid: invalid, blocking: blocking) do |file| file.each_line(chomp: chomp) do |line| yield line end @@ -788,9 +792,9 @@ 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) : Array(String) + def self.read_lines(filename : Path | String, encoding = nil, invalid = nil, chomp = true, blocking = true) : Array(String) lines = [] of String - each_line(filename, encoding: encoding, invalid: invalid, chomp: chomp) do |line| + each_line(filename, encoding: encoding, invalid: invalid, chomp: chomp, blocking: blocking) do |line| lines << line end lines @@ -813,8 +817,8 @@ 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") - open(filename, mode, perm, encoding: encoding, invalid: invalid) do |file| + def self.write(filename : Path | String, content, perm = DEFAULT_CREATE_PERMISSIONS, encoding = nil, invalid = nil, mode = "w", blocking = true) + open(filename, mode, perm, encoding: encoding, invalid: invalid, blocking: blocking) do |file| case content when Bytes file.sync = true From 23d99d88504e3e21f2d2d166ef4a5afd6191f351 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 29 Jan 2024 11:23:44 +0100 Subject: [PATCH 4/7] fixup! Require explicit blocking parameter to File --- src/crystal/system/file.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crystal/system/file.cr b/src/crystal/system/file.cr index 1733acefdc08..89d29512c28e 100644 --- a/src/crystal/system/file.cr +++ b/src/crystal/system/file.cr @@ -65,7 +65,7 @@ module Crystal::System::File io << suffix end - fd, errno = open(path, mode, perm, blocking: true) + fd, errno = open(path, mode, perm) if errno.none? return {fd, path} From 5cd6e7ae4dad1911b15f81ca4a98a4c5ad3702fc Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 29 Jan 2024 12:38:56 +0100 Subject: [PATCH 5/7] Fix: TTY may not be available (e.g. Docker on CI) --- spec/std/file_spec.cr | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/std/file_spec.cr b/spec/std/file_spec.cr index 5b2c848bfb7a..14029424e005 100644 --- a/spec/std/file_spec.cr +++ b/spec/std/file_spec.cr @@ -74,6 +74,8 @@ describe "File" do File.open("/dev/tty", "r", blocking: nil) do |file| file.blocking.should be_false end + rescue File::Error + # The TTY may not be available (e.g. Docker CI) end end From 60cef61b70b1fed3ba4d218603ab8215100ba79d Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Tue, 6 Feb 2024 10:10:23 +0100 Subject: [PATCH 6/7] Fix: don't run thread specs with the interpreter Starting threads very likely requires support from the interpreter to create the thread (so it knows about it) to run the interpreted code in. This also fixes the "can't resume running fiber" exceptions that started appearing in the wait group pull request, because they were always run after the thread related specs. --- spec/interpreter_std_spec.cr | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/interpreter_std_spec.cr b/spec/interpreter_std_spec.cr index 3cdcc55cbd61..82e95272a745 100644 --- a/spec/interpreter_std_spec.cr +++ b/spec/interpreter_std_spec.cr @@ -234,9 +234,9 @@ require "./std/system_error_spec.cr" require "./std/system/group_spec.cr" # require "./std/system_spec.cr" (failed to run) require "./std/system/user_spec.cr" -require "./std/thread/condition_variable_spec.cr" -require "./std/thread/mutex_spec.cr" -# require "./std/thread_spec.cr" (failed to run) +# require "./std/thread/condition_variable_spec.cr" (interpreter must support threads) +# require "./std/thread/mutex_spec.cr" (interpreter must support threads) +# require "./std/thread_spec.cr" (interpreter must support threads) require "./std/time/custom_formats_spec.cr" require "./std/time/format_spec.cr" require "./std/time/location_spec.cr" From a5d8f64b19d0ac2419bef1e2e1576c5871d0c7e2 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Tue, 6 Feb 2024 18:07:29 +0100 Subject: [PATCH 7/7] Fix: disable thread spec when interpreted --- spec/std/file_spec.cr | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/std/file_spec.cr b/spec/std/file_spec.cr index 14029424e005..1e9a9f53ae32 100644 --- a/spec/std/file_spec.cr +++ b/spec/std/file_spec.cr @@ -79,7 +79,9 @@ describe "File" do end end - {% if LibC.has_method?(:mkfifo) %} + {% if LibC.has_method?(:mkfifo) && !flag?(:interpreted) %} + # spec is disabled when interpreted because the interpreter doesn't + # support threads it "opens fifo file as non-blocking" do path = File.tempname("chardev") ret = LibC.mkfifo(path, File::DEFAULT_CREATE_PERMISSIONS)