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

Fix: opening/reading from fifo/chardev files are blocking the thread #14255

6 changes: 3 additions & 3 deletions spec/interpreter_std_spec.cr

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 59 additions & 0 deletions spec/std/file_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,65 @@ 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

File.open(path, "w", blocking: nil) do |file|
file.blocking.should be_true
end
end
end

{% 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

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

{% 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)
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

it "reads entire file" do
str = File.read datapath("test_file.txt")
str.should eq("Hello World\n" * 20)
Expand Down
45 changes: 29 additions & 16 deletions src/file.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -157,10 +157,23 @@ 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* 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
fd = Crystal::System::File.open(filename, mode, perm)
new(filename, fd, blocking: true, encoding: encoding, invalid: invalid)
fd = Crystal::System::File.open(filename, mode, perm: perm)
new(filename, fd, blocking: blocking, encoding: encoding, invalid: invalid)
end

getter path : String
Expand Down Expand Up @@ -712,17 +725,17 @@ 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 = true) : self
new filename, mode, perm, encoding, invalid, blocking
end

# Opens the file named by *filename*. If a file is being created, its initial
# permissions may be set using the *perm* parameter. Then given block will be passed the opened
# 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 = true, &)
file = new filename, mode, perm, encoding, invalid, blocking
begin
yield file
ensure
Expand All @@ -736,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
Expand Down Expand Up @@ -765,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
Expand All @@ -779,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
Expand All @@ -804,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
Expand Down
Loading