From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.1 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 54916208EB for ; Mon, 6 Aug 2018 05:35:11 +0000 (UTC) From: Eric Wong To: spew@80x24.org Subject: [PATCH] io.c: make all pipes nonblocking by default Date: Mon, 6 Aug 2018 05:35:11 +0000 Message-Id: <20180806053511.2421-1-e@80x24.org> List-Id: All normal Ruby IO methods (IO#read, IO#gets, IO#write, ...) are all capable of appearing to be "blocking" when presented with a file description with the O_NONBLOCK flag set; so there is little risk of incompatibility within Ruby-using programs. The biggest compatibility risk is when spawning external programs. As a result, stdin, stdout, and stderr are now always made blocking before exec-family calls. Timer-thread elimination in https://bugs.ruby-lang.org/issues/14937 introduced a race condition in signal handling. It is possible to receive a signal inside BLOCKING_REGION right before read/write syscalls. If this patch cannot be accepted, I will have to revert to reintroduce timer-thread and increase resource use (which led to other failures in the past). The race condition introduced for [Misc #14937] led to rare CI failures on a few tests: - test/ruby/test_thread.rb (test_thread_timer_and_interrupt): http://www.rubyist.net/~akr/chkbuild/debian/ruby-trunk/log/20180805T080500Z.fail.html.gz - test/ruby/test_io.rb (test_race_gets_and_close): http://ci.rvm.jp/results/trunk@P895/1190369 This change is ALSO necessary to take advantage of (proposed lightweight concurrency (aka "auto-Fiber") or any similar proposal: https://bugs.ruby-lang.org/issues/13618 TODO: all sockets and FIFOs non-blocking by default, too --- io.c | 44 +++++++++++++----------- process.c | 44 ++++++++++++++++++++++++ spec/ruby/core/io/read_nonblock_spec.rb | 1 - spec/ruby/core/io/write_nonblock_spec.rb | 1 - test/io/nonblock/test_flush.rb | 1 + test/ruby/test_io.rb | 1 + test/ruby/test_process.rb | 8 +++++ thread.c | 31 +++++++++++++---- win32/win32.c | 17 +++++++-- 9 files changed, 116 insertions(+), 32 deletions(-) diff --git a/io.c b/io.c index 1d7626c089..5591b99016 100644 --- a/io.c +++ b/io.c @@ -316,6 +316,26 @@ rb_cloexec_dup2(int oldfd, int newfd) return ret; } +static int +rb_fd_set_nonblock(int fd) +{ +#ifdef _WIN32 + return rb_w32_set_nonblock(fd); +#elif defined(F_GETFL) + int oflags; + int err; + + oflags = fcntl(fd, F_GETFL); + if (oflags == -1) + return -1; + oflags |= O_NONBLOCK; + err = fcntl(fd, F_SETFL, oflags); + if (err == -1) + return -1; +#endif + return 0; +} + int rb_cloexec_pipe(int fildes[2]) { @@ -324,7 +344,7 @@ rb_cloexec_pipe(int fildes[2]) #if defined(HAVE_PIPE2) static int try_pipe2 = 1; if (try_pipe2) { - ret = pipe2(fildes, O_CLOEXEC); + ret = pipe2(fildes, O_CLOEXEC | O_NONBLOCK); if (ret != -1) return ret; /* pipe2 is available since Linux 2.6.27, glibc 2.9. */ @@ -350,6 +370,8 @@ rb_cloexec_pipe(int fildes[2]) #endif rb_maygvl_fd_fix_cloexec(fildes[0]); rb_maygvl_fd_fix_cloexec(fildes[1]); + rb_fd_set_nonblock(fildes[0]); + rb_fd_set_nonblock(fildes[1]); return ret; } @@ -2691,27 +2713,9 @@ read_all(rb_io_t *fptr, long siz, VALUE str) void rb_io_set_nonblock(rb_io_t *fptr) { -#ifdef _WIN32 - if (rb_w32_set_nonblock(fptr->fd) != 0) { + if (rb_fd_set_nonblock(fptr->fd) != 0) { rb_sys_fail_path(fptr->pathv); } -#else - int oflags; -#ifdef F_GETFL - oflags = fcntl(fptr->fd, F_GETFL); - if (oflags == -1) { - rb_sys_fail_path(fptr->pathv); - } -#else - oflags = 0; -#endif - if ((oflags & O_NONBLOCK) == 0) { - oflags |= O_NONBLOCK; - if (fcntl(fptr->fd, F_SETFL, oflags) == -1) { - rb_sys_fail_path(fptr->pathv); - } - } -#endif } struct read_internal_arg { diff --git a/process.c b/process.c index 0c534a8a36..a88eb530a7 100644 --- a/process.c +++ b/process.c @@ -1450,6 +1450,45 @@ before_exec_non_async_signal_safe(void) rb_thread_stop_timer_thread(); } +#define WRITE_CONST(fd, str) (void)(write((fd),(str),sizeof(str)-1)<0) +#ifdef _WIN32 +int rb_w32_set_nonblock2(int fd, int nonblock); +#endif + +static int +set_blocking(int fd) +{ +#ifdef _WIN32 + return rb_w32_set_nonblock2(fd, 0); +#elif defined(F_GETFL) && defined(F_SETFL) + int fl = fcntl(fd, F_GETFL); /* async-signal-safe */ + + /* EBADF ought to be possible */ + if (fl == -1) return fl; + if (fl & O_NONBLOCK) { + fl &= ~O_NONBLOCK; + return fcntl(fd, F_SETFL, fl); + } + return 0; +#endif +} + +static void +stdfd_clear_nonblock(void) +{ + /* many programs cannot deal with non-blocking stdin/stdout/stderr */ + int fd; + for (fd = 0; fd < 3; fd++) { + if (set_blocking(fd) != 0) { + int e = errno; + + if (e != EBADF) { + rb_async_bug_errno("set_blocking failed before exec", e); + } + } + } +} + static void before_exec(void) { @@ -3421,6 +3460,7 @@ rb_execarg_run_options(const struct rb_execarg *eargp, struct rb_execarg *sargp, rb_execarg_allocate_dup2_tmpbuf(sargp, RARRAY_LEN(ary)); } } + stdfd_clear_nonblock(); return 0; } @@ -3621,6 +3661,10 @@ read_retry(int fd, void *buf, size_t len) { ssize_t r; + if (set_blocking(fd) != 0) { + rb_async_bug_errno("set_blocking failed reading child error", errno); + } + do { r = read(fd, buf, len); } while (r < 0 && errno == EINTR); diff --git a/spec/ruby/core/io/read_nonblock_spec.rb b/spec/ruby/core/io/read_nonblock_spec.rb index e224707e38..3c02f662f6 100644 --- a/spec/ruby/core/io/read_nonblock_spec.rb +++ b/spec/ruby/core/io/read_nonblock_spec.rb @@ -44,7 +44,6 @@ platform_is_not :windows do it 'sets the IO in nonblock mode' do require 'io/nonblock' - @read.nonblock?.should == false @write.write "abc" @read.read_nonblock(1).should == "a" @read.nonblock?.should == true diff --git a/spec/ruby/core/io/write_nonblock_spec.rb b/spec/ruby/core/io/write_nonblock_spec.rb index b0da9b7e11..285d1af376 100644 --- a/spec/ruby/core/io/write_nonblock_spec.rb +++ b/spec/ruby/core/io/write_nonblock_spec.rb @@ -76,7 +76,6 @@ platform_is_not :windows do it 'sets the IO in nonblock mode' do require 'io/nonblock' - @write.nonblock?.should == false @write.write_nonblock('a') @write.nonblock?.should == true end diff --git a/test/io/nonblock/test_flush.rb b/test/io/nonblock/test_flush.rb index 63e16db5a3..08d129de3f 100644 --- a/test/io/nonblock/test_flush.rb +++ b/test/io/nonblock/test_flush.rb @@ -53,6 +53,7 @@ def flush_test(r, w) def test_nonblock IO.pipe {|r, w| + w.nonblock = false assert_equal(false, w.nonblock?) w.nonblock do assert_equal(true, w.nonblock?) diff --git a/test/ruby/test_io.rb b/test/ruby/test_io.rb index a1462c53b2..2e0b12d435 100644 --- a/test/ruby/test_io.rb +++ b/test/ruby/test_io.rb @@ -1361,6 +1361,7 @@ def test_readpartial def test_readpartial_lock with_pipe do |r, w| s = "" + r.nonblock = false t = Thread.new { r.readpartial(5, s) } Thread.pass until t.stop? assert_raise(RuntimeError) { s.clear } diff --git a/test/ruby/test_process.rb b/test/ruby/test_process.rb index 759230f834..58a5709562 100644 --- a/test/ruby/test_process.rb +++ b/test/ruby/test_process.rb @@ -762,6 +762,14 @@ def test_execopts_redirect_pipe Process.wait pid end } + + # ensure standard FDs we redirect to are blocking for compatibility + with_pipes(4) do |pipes| + src = 'p [STDIN,STDOUT,STDERR].map(&:nonblock?)' + rdr = { 0 => pipes[0][0], 1 => pipes[1][1], 2 => pipes[2][1] } + pid = spawn(RUBY, '-rio/nonblock', '-e', src, rdr) + assert_equal("[false, false, false]\n", pipes[1][0].gets) + end end end diff --git a/thread.c b/thread.c index a7d48a464f..331362c576 100644 --- a/thread.c +++ b/thread.c @@ -4031,8 +4031,11 @@ rb_wait_for_single_fd(int fd, int events, struct timeval *timeout) struct pollfd fds; int result = 0, lerrno; struct timespec ts, end, *tsp; - rb_thread_t *th = GET_THREAD(); + struct waiting_fd wfd; + enum ruby_tag_type state; + wfd.th = GET_THREAD(); + wfd.fd = fd; timeout_prepare(&tsp, &ts, &end, timeout); fds.fd = fd; fds.events = (short)events; @@ -4040,12 +4043,21 @@ rb_wait_for_single_fd(int fd, int events, struct timeval *timeout) do { fds.revents = 0; lerrno = 0; - BLOCKING_REGION(th, { - result = ppoll(&fds, 1, tsp, NULL); - if (result < 0) lerrno = errno; - }, ubf_select, th, FALSE); - - RUBY_VM_CHECK_INTS_BLOCKING(th->ec); + list_add(&wfd.th->vm->waiting_fds, &wfd.wfd_node); + + EC_PUSH_TAG(wfd.th->ec); + if ((state = EC_EXEC_TAG()) == TAG_NONE) { + BLOCKING_REGION(wfd.th, { + result = ppoll(&fds, 1, tsp, NULL); + if (result < 0) lerrno = errno; + }, ubf_select, wfd.th, FALSE); + } + EC_POP_TAG(); + list_del(&wfd.wfd_node); + if (state) { + EC_JUMP_TAG(wfd.th->ec, state); + } + RUBY_VM_CHECK_INTS_BLOCKING(wfd.th->ec); } while (wait_retryable(&result, lerrno, tsp, &end)); if (result < 0) { errno = lerrno; @@ -4096,6 +4108,7 @@ struct select_args { rb_fdset_t *read; rb_fdset_t *write; rb_fdset_t *except; + struct waiting_fd wfd; struct timeval *tv; }; @@ -4126,6 +4139,7 @@ select_single_cleanup(VALUE ptr) { struct select_args *args = (struct select_args *)ptr; + list_del(&args->wfd.wfd_node); if (args->read) rb_fd_term(args->read); if (args->write) rb_fd_term(args->write); if (args->except) rb_fd_term(args->except); @@ -4146,7 +4160,10 @@ rb_wait_for_single_fd(int fd, int events, struct timeval *tv) args.write = (events & RB_WAITFD_OUT) ? init_set_fd(fd, &wfds) : NULL; args.except = (events & RB_WAITFD_PRI) ? init_set_fd(fd, &efds) : NULL; args.tv = tv; + args.wfd.fd = fd; + args.wfd.th = GET_THREAD(); + list_add(&args.wfd.th->vm->waiting_fds, &args.wfd.wfd_node); r = (int)rb_ensure(select_single, ptr, select_single_cleanup, ptr); if (r == -1) errno = args.as.error; diff --git a/win32/win32.c b/win32/win32.c index 52de2cc63d..94e052dd48 100644 --- a/win32/win32.c +++ b/win32/win32.c @@ -4389,11 +4389,11 @@ fcntl(int fd, int cmd, ...) /* License: Ruby's */ int -rb_w32_set_nonblock(int fd) +rb_w32_set_nonblock2(int fd, int nonblock) { SOCKET sock = TO_SOCKET(fd); if (is_socket(sock)) { - return setfl(sock, O_NONBLOCK); + return setfl(sock, nonblock ? O_NONBLOCK : 0); } else if (is_pipe(sock)) { DWORD state; @@ -4401,7 +4401,12 @@ rb_w32_set_nonblock(int fd) errno = map_errno(GetLastError()); return -1; } - state |= PIPE_NOWAIT; + if (nonblock) { + state |= PIPE_NOWAIT; + } + else { + state &= ~PIPE_NOWAIT; + } if (!SetNamedPipeHandleState((HANDLE)sock, &state, NULL, NULL)) { errno = map_errno(GetLastError()); return -1; @@ -4414,6 +4419,12 @@ rb_w32_set_nonblock(int fd) } } +int +rb_w32_set_nonblock(int fd) +{ + return rb_w32_set_nonblock2(fd, TRUE); +} + #ifndef WNOHANG #define WNOHANG -1 #endif -- EW