From 318148843f676694d5e2f6a227e1eebc12402212 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Mon, 28 Aug 2023 21:08:59 +0200 Subject: [PATCH] Reimplement IO.select with poll(2) * This avoids the issue of file descriptors >= 1024 that FD_ZERO does not handle. * Fixes https://github.com/oracle/truffleruby/issues/3201 * Skip TestIO#test_select_exceptfds on macOS, macOS's poll(2) is buggy with TCP MSG_OOB or macOS does not really support MSG_OOB: For a fd to which out-of-band data has been sent, poll() with POLLIN|POLLPRI returns POLLRDNORM|POLLPRI|POLLIN on Linux (correct) but POLLRDNORM|POLLIN on macOS (bug). poll() with just POLLPRI works fine on Linux but hangs on macOS (bug). It seems a bug of macOS poll() not handling TCP MSG_OOB. The man page of poll() on macOS talks about: "The distinction between normal, priority, and high-priority data is specific to particular file types or devices." but gives no details, it seems a mess. MSG_OOB is poorly supported across platforms anyway and extremely rarely if ever used. --- CHANGELOG.md | 1 + spec/ruby/core/io/select_spec.rb | 15 +++- spec/truffle/io/read_spec.rb | 10 +-- src/main/c/truffleposix/truffleposix.c | 49 +---------- .../DarwinAArch64NativeConfiguration.java | 6 ++ .../DarwinAMD64NativeConfiguration.java | 6 ++ .../LinuxAArch64NativeConfiguration.java | 6 ++ .../LinuxAMD64NativeConfiguration.java | 6 ++ src/main/ruby/truffleruby/core/io.rb | 8 +- src/main/ruby/truffleruby/core/posix.rb | 10 +-- .../truffleruby/core/truffle/io_operations.rb | 85 ++++++++++++------- test/mri/excludes/TestIO.rb | 1 + tool/generate-native-config.rb | 2 +- 13 files changed, 108 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34eabae674d..5c0bd35e728 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Bug fixes: * Fix `super` method lookup for unbounded attached methods (#3131, @itarato). * Fix `Module#define_method(name, Method)` to respect `module_function` visibility (#3181, @andrykonchin). * Fix stack overflow with `Kernel.require` and `zeitwerk` (#3224, @eregon). +* Reimplement `IO.select` with `poll(2)` to support file descriptors >= 1024 (#3201, @eregon). Compatibility: diff --git a/spec/ruby/core/io/select_spec.rb b/spec/ruby/core/io/select_spec.rb index 4603c1fbbca..1e4e50a81b7 100644 --- a/spec/ruby/core/io/select_spec.rb +++ b/spec/ruby/core/io/select_spec.rb @@ -55,8 +55,8 @@ end end - it "returns supplied objects correctly even when monitoring the same object in different arrays" do - filename = tmp("IO_select_pipe_file") + $$.to_s + it "returns supplied objects correctly when monitoring the same object in different arrays" do + filename = tmp("IO_select_pipe_file") io = File.open(filename, 'w+') result = IO.select [io], [io], nil, 0 result.should == [[io], [io], []] @@ -64,6 +64,17 @@ rm_r filename end + it "returns the pipe read end in read set if the pipe write end is closed concurrently" do + main = Thread.current + t = Thread.new { + Thread.pass until main.stop? + @wr.close + } + IO.select([@rd]).should == [[@rd], [], []] + ensure + t.join + end + it "invokes to_io on supplied objects that are not IO and returns the supplied objects" do # make some data available @wr.write("foobar") diff --git a/spec/truffle/io/read_spec.rb b/spec/truffle/io/read_spec.rb index 84fdd3f34b2..e091e9d467c 100644 --- a/spec/truffle/io/read_spec.rb +++ b/spec/truffle/io/read_spec.rb @@ -29,10 +29,10 @@ w.write "b" w.flush - select_ignoring_iobuffer([r], 1_000_000).should == [r] + select_ignoring_iobuffer([r], 100).should == [r] r.read(1).should == "a" - select_ignoring_iobuffer([r], 1_000_000).should == [r] + select_ignoring_iobuffer([r], 1000).should == [r] r.read(1).should == "b" end @@ -42,17 +42,17 @@ @w.flush @r.gets.should == "a\n" - select_ignoring_iobuffer([@r], 100_000).should == nil + select_ignoring_iobuffer([@r], 100).should == nil end - def select_ignoring_iobuffer(ios, timeout_us) + def select_ignoring_iobuffer(ios, timeout_ms) return IO.select(ios)[0] unless defined?(::TruffleRuby) result = Truffle::IOOperations.select( ios, ios, [], [], [], [], - timeout_us, timeout_us) + timeout_ms) result and result[0] end end diff --git a/src/main/c/truffleposix/truffleposix.c b/src/main/c/truffleposix/truffleposix.c index 5a6702c9e7d..1fa3c6e2ef1 100644 --- a/src/main/c/truffleposix/truffleposix.c +++ b/src/main/c/truffleposix/truffleposix.c @@ -60,7 +60,6 @@ SUCH DAMAGE. #include #include #include -#include #include #include #include @@ -91,27 +90,7 @@ struct truffleposix_stat { static void copy_stat(struct stat *stat, struct truffleposix_stat* buffer); -static void init_fd_set(fd_set *set, int nfds, int *fds, int *maxfd) { - FD_ZERO(set); - for (int i = 0; i < nfds; i++) { - int fd = fds[i]; - FD_SET(fd, set); - if (fd > *maxfd) { - *maxfd = fd; - } - } -} - -static void mark_ready_from_set(fd_set *set, int nfds, int *fds) { - for (int i = 0; i < nfds; i++) { - int fd = fds[i]; - if (!FD_ISSET(fd, set)) { - fds[i] = -1; - } - } -} - -int truffleposix_poll(int fd, int events, int timeout_ms) { +int truffleposix_poll_single_fd(int fd, int events, int timeout_ms) { struct pollfd fds; fds.fd = fd; @@ -120,32 +99,6 @@ int truffleposix_poll(int fd, int events, int timeout_ms) { return poll(&fds, 1, timeout_ms) >= 0 ? fds.revents : -1; } -int truffleposix_select(int nread, int *readfds, int nwrite, int *writefds, - int nexcept, int *exceptfds, long timeout_us) { - struct timeval timeout; - struct timeval *timeout_ptr = NULL; - - if (timeout_us >= 0) { - timeout.tv_sec = (timeout_us / 1000000); - timeout.tv_usec = (timeout_us % 1000000); - timeout_ptr = &timeout; - } - - int maxfd = 0; - fd_set readset, writeset, exceptset; - init_fd_set(&readset, nread, readfds, &maxfd); - init_fd_set(&writeset, nwrite, writefds, &maxfd); - init_fd_set(&exceptset, nexcept, exceptfds, &maxfd); - - int ret = select(maxfd+1, &readset, &writeset, &exceptset, timeout_ptr); - if (ret > 0) { - mark_ready_from_set(&readset, nread, readfds); - mark_ready_from_set(&writeset, nwrite, writefds); - mark_ready_from_set(&exceptset, nexcept, exceptfds); - } - return ret; -} - int truffleposix_utimes(const char *filename, long atime_sec, int atime_nsec, long mtime_sec, int mtime_nsec) { struct timespec timespecs[2]; diff --git a/src/main/java/org/truffleruby/platform/DarwinAArch64NativeConfiguration.java b/src/main/java/org/truffleruby/platform/DarwinAArch64NativeConfiguration.java index ff516fecfa9..7a1cfc80111 100644 --- a/src/main/java/org/truffleruby/platform/DarwinAArch64NativeConfiguration.java +++ b/src/main/java/org/truffleruby/platform/DarwinAArch64NativeConfiguration.java @@ -407,6 +407,12 @@ public static void load(NativeConfiguration configuration, RubyContext context) configuration.config("platform.poll.POLLIN", 1); configuration.config("platform.poll.POLLPRI", 2); configuration.config("platform.poll.POLLOUT", 4); + configuration.config("platform.poll.POLLERR", 8); + configuration.config("platform.poll.POLLHUP", 16); + configuration.config("platform.poll.POLLRDNORM", 64); + configuration.config("platform.poll.POLLRDBAND", 128); + configuration.config("platform.poll.POLLWRNORM", 4); + configuration.config("platform.poll.POLLWRBAND", 256); configuration.config("platform.socket.AF_APPLETALK", 16); configuration.config("platform.socket.PF_APPLETALK", 16); configuration.config("platform.socket.AF_INET", 2); diff --git a/src/main/java/org/truffleruby/platform/DarwinAMD64NativeConfiguration.java b/src/main/java/org/truffleruby/platform/DarwinAMD64NativeConfiguration.java index ca0116d2ef4..50754792b03 100644 --- a/src/main/java/org/truffleruby/platform/DarwinAMD64NativeConfiguration.java +++ b/src/main/java/org/truffleruby/platform/DarwinAMD64NativeConfiguration.java @@ -407,6 +407,12 @@ public static void load(NativeConfiguration configuration, RubyContext context) configuration.config("platform.poll.POLLIN", 1); configuration.config("platform.poll.POLLPRI", 2); configuration.config("platform.poll.POLLOUT", 4); + configuration.config("platform.poll.POLLERR", 8); + configuration.config("platform.poll.POLLHUP", 16); + configuration.config("platform.poll.POLLRDNORM", 64); + configuration.config("platform.poll.POLLRDBAND", 128); + configuration.config("platform.poll.POLLWRNORM", 4); + configuration.config("platform.poll.POLLWRBAND", 256); configuration.config("platform.socket.AF_APPLETALK", 16); configuration.config("platform.socket.PF_APPLETALK", 16); configuration.config("platform.socket.AF_INET", 2); diff --git a/src/main/java/org/truffleruby/platform/LinuxAArch64NativeConfiguration.java b/src/main/java/org/truffleruby/platform/LinuxAArch64NativeConfiguration.java index 7e37c48b09c..3e19ae23e7a 100644 --- a/src/main/java/org/truffleruby/platform/LinuxAArch64NativeConfiguration.java +++ b/src/main/java/org/truffleruby/platform/LinuxAArch64NativeConfiguration.java @@ -418,6 +418,12 @@ public static void load(NativeConfiguration configuration, RubyContext context) configuration.config("platform.poll.POLLIN", 1); configuration.config("platform.poll.POLLPRI", 2); configuration.config("platform.poll.POLLOUT", 4); + configuration.config("platform.poll.POLLERR", 8); + configuration.config("platform.poll.POLLHUP", 16); + configuration.config("platform.poll.POLLRDNORM", 64); + configuration.config("platform.poll.POLLRDBAND", 128); + configuration.config("platform.poll.POLLWRNORM", 256); + configuration.config("platform.poll.POLLWRBAND", 512); configuration.config("platform.socket.AF_APPLETALK", 5); configuration.config("platform.socket.PF_APPLETALK", 5); configuration.config("platform.socket.AF_AX25", 3); diff --git a/src/main/java/org/truffleruby/platform/LinuxAMD64NativeConfiguration.java b/src/main/java/org/truffleruby/platform/LinuxAMD64NativeConfiguration.java index 3e8ba50c4fa..78dd9ebb755 100644 --- a/src/main/java/org/truffleruby/platform/LinuxAMD64NativeConfiguration.java +++ b/src/main/java/org/truffleruby/platform/LinuxAMD64NativeConfiguration.java @@ -418,6 +418,12 @@ public static void load(NativeConfiguration configuration, RubyContext context) configuration.config("platform.poll.POLLIN", 1); configuration.config("platform.poll.POLLPRI", 2); configuration.config("platform.poll.POLLOUT", 4); + configuration.config("platform.poll.POLLERR", 8); + configuration.config("platform.poll.POLLHUP", 16); + configuration.config("platform.poll.POLLRDNORM", 64); + configuration.config("platform.poll.POLLRDBAND", 128); + configuration.config("platform.poll.POLLWRNORM", 256); + configuration.config("platform.poll.POLLWRBAND", 512); configuration.config("platform.socket.AF_APPLETALK", 5); configuration.config("platform.socket.PF_APPLETALK", 5); configuration.config("platform.socket.AF_AX25", 3); diff --git a/src/main/ruby/truffleruby/core/io.rb b/src/main/ruby/truffleruby/core/io.rb index 1fa4efb5c29..5a25c0338f6 100644 --- a/src/main/ruby/truffleruby/core/io.rb +++ b/src/main/ruby/truffleruby/core/io.rb @@ -735,10 +735,8 @@ def self.select(readables = nil, writables = nil, errorables = nil, timeout = ni raise ArgumentError, 'timeout must be positive' if timeout < 0 - # Microseconds, rounded down - timeout = remaining_timeout = Integer(timeout * 1_000_000) - else - remaining_timeout = -1 + # Milliseconds, rounded down + timeout = Integer(timeout * 1_000) end if readables @@ -782,7 +780,7 @@ def self.select(readables = nil, writables = nil, errorables = nil, timeout = ni readables, readable_ios, writables, writable_ios, errorables, errorable_ios, - timeout, remaining_timeout) + timeout) end ## diff --git a/src/main/ruby/truffleruby/core/posix.rb b/src/main/ruby/truffleruby/core/posix.rb index 5de7668ec74..2668254b5f1 100644 --- a/src/main/ruby/truffleruby/core/posix.rb +++ b/src/main/ruby/truffleruby/core/posix.rb @@ -214,7 +214,9 @@ def self.attach_function_eagerly(native_name, argument_types, return_type, attach_function :open, [:string, :int, varargs(:mode_t)], :int attach_function :opendir, [:string], :pointer attach_function :pipe, [:pointer], :int - attach_function :truffleposix_poll, [:pointer, :long, :int], :int, LIBTRUFFLEPOSIX, true + # blocking=false for both poll because the timeout needs to be decreased on EINTR + attach_function :truffleposix_poll_single_fd, [:pointer, :long, :int], :int, LIBTRUFFLEPOSIX + attach_function :poll, [:pointer, :long, :int], :int attach_function :read, [:int, :pointer, :size_t], :ssize_t, LIBC, true attach_function :readlink, [:string, :pointer, :size_t], :ssize_t attach_function :realpath, [:string, :pointer], :pointer @@ -224,8 +226,6 @@ def self.attach_function_eagerly(native_name, argument_types, return_type, attach_function :truffleposix_rewinddir, [:pointer], :void, LIBTRUFFLEPOSIX attach_function :rmdir, [:string], :int attach_function :seekdir, [:pointer, :long], :void - select_args = [:int, :pointer, :int, :pointer, :int, :pointer, :long] - attach_function :truffleposix_select, select_args, :int, LIBTRUFFLEPOSIX attach_function :truffleposix_stat, [:string, :pointer], :int, LIBTRUFFLEPOSIX attach_function :truffleposix_stat_mode, [:string], :mode_t, LIBTRUFFLEPOSIX attach_function :truffleposix_stat_size, [:string], :long, LIBTRUFFLEPOSIX @@ -240,8 +240,8 @@ def self.attach_function_eagerly(native_name, argument_types, return_type, Truffle::Boot.delay do if NATIVE # We should capture the non-lazy method - attach_function_eagerly :truffleposix_select, select_args, :int, LIBTRUFFLEPOSIX, false, :truffleposix_select, self - SELECT = method(:truffleposix_select) + attach_function_eagerly :poll, [:pointer, :long, :int], :int, LIBC, false, :poll, self + POLL = method(:poll) end end diff --git a/src/main/ruby/truffleruby/core/truffle/io_operations.rb b/src/main/ruby/truffleruby/core/truffle/io_operations.rb index cedb79dd8be..e49f89aa742 100644 --- a/src/main/ruby/truffleruby/core/truffle/io_operations.rb +++ b/src/main/ruby/truffleruby/core/truffle/io_operations.rb @@ -14,6 +14,18 @@ module IOOperations POLLIN = Truffle::Config['platform.poll.POLLIN'] POLLPRI = Truffle::Config['platform.poll.POLLPRI'] POLLOUT = Truffle::Config['platform.poll.POLLOUT'] + POLLERR = Truffle::Config['platform.poll.POLLERR'] + POLLHUP = Truffle::Config['platform.poll.POLLHUP'] + POLLRDNORM = Truffle::Config['platform.poll.POLLRDNORM'] + POLLRDBAND = Truffle::Config['platform.poll.POLLRDBAND'] + POLLWRNORM = Truffle::Config['platform.poll.POLLWRNORM'] + POLLWRBAND = Truffle::Config['platform.poll.POLLWRBAND'] + + # POLLIN, POLLOUT have a different meanings from select(2)'s read/write bit. + # See thread.c in CRuby, the definitions are from there. + POLLIN_SET = POLLRDNORM | POLLRDBAND | POLLIN | POLLHUP | POLLERR + POLLOUT_SET = POLLWRBAND | POLLWRNORM | POLLOUT | POLLERR + POLLEX_SET = POLLPRI def self.print(io, args, last_line_storage) if args.empty? @@ -132,53 +144,64 @@ def self.create_pipe(read_class, write_class, external = nil, internal = nil) end SIZEOF_INT = FFI::Pointer.find_type_size(:int) + SIZEOF_SHORT = FFI::Pointer.find_type_size(:short) + SIZEOF_STRUCT_POLLFD = SIZEOF_INT + 2 * SIZEOF_SHORT - def self.to_fds(ios, pointer) + def self.to_fds(ios, pointer, events) ios.each_with_index do |io, i| - pointer.put_int(i * SIZEOF_INT, io.fileno) + offset = i * SIZEOF_STRUCT_POLLFD + pointer.put_int(offset, io.fileno) # file descriptor + pointer.put_short(offset + SIZEOF_INT, events) # requested events + pointer.put_short(offset + SIZEOF_INT + SIZEOF_SHORT, 0) # returned events end end - def self.mark_ready(objects, pointer) + def self.mark_ready(ios, pointer, event) ready = [] - pointer.read_array_of_int(objects.size).each_with_index do |fd, i| - ready << objects[i] if fd >= 0 + ios.each_with_index do |io, i| + offset = i * SIZEOF_STRUCT_POLLFD + returned_events = pointer.get_short(offset + SIZEOF_INT + SIZEOF_SHORT) + ready << io if returned_events.anybits?(event) end ready end - def self.select(readables, readable_ios, writables, writable_ios, errorables, errorable_ios, timeout, remaining_timeout) + def self.select(readables, readable_ios, writables, writable_ios, errorables, errorable_ios, timeout) if timeout - start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) + while timeout > 2147483647 # INT_MAX + timeout -= 2147483000 + ret = select(readables, readable_ios, writables, writable_ios, errorables, errorable_ios, 2147483000) + return ret unless Primitive.nil?(ret) + end + + remaining_timeout = timeout + start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond) + deadline = start + timeout + else + remaining_timeout = -1 # infinite timeout end + total_size = readables.size + writables.size + errorables.size buffer, readables_pointer, writables_pointer, errorables_pointer = - Truffle::FFI::Pool.stack_alloc(readables.size * SIZEOF_INT, writables.size * SIZEOF_INT, errorables.size * SIZEOF_INT) + Truffle::FFI::Pool.stack_alloc(readables.size * SIZEOF_STRUCT_POLLFD, writables.size * SIZEOF_STRUCT_POLLFD, errorables.size * SIZEOF_STRUCT_POLLFD) begin begin - to_fds(readable_ios, readables_pointer) - to_fds(writable_ios, writables_pointer) - to_fds(errorable_ios, errorables_pointer) - - primitive_result = Primitive.thread_run_blocking_nfi_system_call(Truffle::POSIX::SELECT, [ - readables.size, readables_pointer, - writables.size, writables_pointer, - errorables.size, errorables_pointer, - remaining_timeout - ]) + to_fds(readable_ios, readables_pointer, POLLIN) + to_fds(writable_ios, writables_pointer, POLLOUT) + to_fds(errorable_ios, errorables_pointer, POLLPRI) + primitive_result = Primitive.thread_run_blocking_nfi_system_call(Truffle::POSIX::POLL, [buffer, total_size, remaining_timeout]) result = if primitive_result < 0 errno = Errno.errno if errno == Errno::EINTR::Errno if timeout # Update timeout - now = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - waited = now - start - if waited >= timeout + now = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond) + if now >= deadline nil # timeout else - remaining_timeout = timeout - waited + remaining_timeout = deadline - now :retry end else @@ -195,9 +218,9 @@ def self.select(readables, readable_ios, writables, writable_ios, errorables, er if result == 0 nil # timeout else - [mark_ready(readables, readables_pointer), - mark_ready(writables, writables_pointer), - mark_ready(errorables, errorables_pointer)] + [mark_ready(readables, readables_pointer, POLLIN_SET), + mark_ready(writables, writables_pointer, POLLOUT_SET), + mark_ready(errorables, errorables_pointer, POLLEX_SET)] end ensure Truffle::FFI::Pool.stack_free(buffer) @@ -205,8 +228,8 @@ def self.select(readables, readable_ios, writables, writable_ios, errorables, er end - # This method will return an event mask if poll returned without error. - # The event mask is > 0 when an event occurred within the timeout, 0 if the timeout expired. + # This method will return an event mask (an Integer). + # The returned value is > 0 when an event occurred within the timeout and 0 if the timeout expired with no events. # Raises an exception for an errno. def self.poll(io, event_mask, timeout) if (event_mask & POLLIN) != 0 @@ -236,16 +259,16 @@ def self.poll(io, event_mask, timeout) end begin - primitive_result = Truffle::POSIX.truffleposix_poll(Primitive.io_fd(io), event_mask, remaining_timeout) + returned_events = Truffle::POSIX.truffleposix_poll_single_fd(Primitive.io_fd(io), event_mask, remaining_timeout) result = - if primitive_result < 0 + if returned_events < 0 errno = Errno.errno if errno == Errno::EINTR::Errno if timeout_ms # Update timeout now = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond) if now >= deadline - false # timeout + 0 # timeout else remaining_timeout = deadline - now :retry @@ -257,7 +280,7 @@ def self.poll(io, event_mask, timeout) Errno.handle_errno(errno) end else - primitive_result + returned_events end end while result == :retry diff --git a/test/mri/excludes/TestIO.rb b/test/mri/excludes/TestIO.rb index 1ccd06372ab..4984c53dee2 100644 --- a/test/mri/excludes/TestIO.rb +++ b/test/mri/excludes/TestIO.rb @@ -65,3 +65,4 @@ exclude :test_each_line, "[Bug #18770]." exclude :test_explicit_path, "Expected /Fake Path/ to match \"#\"." exclude :test_dup_timeout, "NoMethodError: undefined method `timeout=' for #" +exclude :test_select_exceptfds, "Truffle::IOOperations.poll(acc, Truffle::IOOperations::POLLIN_SET | Truffle::IOOperations::POLLEX_SET, nil) returns POLLRDNORM|POLLPRI|POLLIN on Linux (correct) but POLLRDNORM|POLLIN on macOS (bug). With events=POLLPRI it hangs on macOS. It seems a bug of macOS poll() not handling TCP MSG_OOB. MSG_OOB is poorly supported across platforms anyway." if RUBY_PLATFORM.include?('darwin') diff --git a/tool/generate-native-config.rb b/tool/generate-native-config.rb index 9b81da6c92e..f5d531bc808 100644 --- a/tool/generate-native-config.rb +++ b/tool/generate-native-config.rb @@ -640,7 +640,7 @@ def constants(name) constants 'poll' do |cg| cg.include 'poll.h' cg.consts %w[ - POLLIN POLLPRI POLLOUT + POLLIN POLLPRI POLLOUT POLLERR POLLHUP POLLRDNORM POLLRDBAND POLLWRNORM POLLWRBAND ] end