io.c: wait on FD readability w/o GVL reacquisition

Since non-blocking I/O is the default after [Bug #14968],
we will hit it more often and cause more acquisition/release
of GVL to wait on single FD.

This also lets us avoid touching the temporal string locking
as much and lets us clean up some test changes made for
[Bug #14968]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@65948 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

2 files changed, 47 insertions, 38 deletions

diff --git a/io.c b/io.c
index 680659e333..da3fcfc566 100644
--- a/io.c
+++ b/io.c
@@ -944,6 +944,7 @@ io_alloc(VALUE klass)
 
 struct io_internal_read_struct {
     int fd;
+    int nonblock;
     void *buf;
     size_t capa;
 };
@@ -962,11 +963,24 @@ struct io_internal_writev_struct {
 };
 #endif
 
+static int nogvl_wait_for_single_fd(int fd, short events);
 static VALUE
 internal_read_func(void *ptr)
 {
     struct io_internal_read_struct *iis = ptr;
-    return read(iis->fd, iis->buf, iis->capa);
+    ssize_t r;
+retry:
+    r = read(iis->fd, iis->buf, iis->capa);
+    if (r < 0 && !iis->nonblock) {
+        int e = errno;
+        if (e == EAGAIN || e == EWOULDBLOCK) {
+            if (nogvl_wait_for_single_fd(iis->fd, RB_WAITFD_IN) != -1) {
+                goto retry;
+            }
+            errno = e;
+        }
+    }
+    return r;
 }
 
 #if defined __APPLE__
@@ -1004,7 +1018,9 @@ static ssize_t
 rb_read_internal(int fd, void *buf, size_t count)
 {
     struct io_internal_read_struct iis;
+
     iis.fd = fd;
+    iis.nonblock = 0;
     iis.buf = buf;
     iis.capa = count;
 
@@ -2735,18 +2751,18 @@ rb_io_set_nonblock(rb_io_t *fptr)
     }
 }
 
-struct read_internal_arg {
-    int fd;
-    char *str_ptr;
-    long len;
-};
-
 static VALUE
 read_internal_call(VALUE arg)
 {
-    struct read_internal_arg *p = (struct read_internal_arg *)arg;
-    p->len = rb_read_internal(p->fd, p->str_ptr, p->len);
-    return Qundef;
+    struct io_internal_read_struct *iis = (struct io_internal_read_struct *)arg;
+
+    return rb_thread_io_blocking_region(internal_read_func, iis, iis->fd);
+}
+
+static long
+read_internal_locktmp(VALUE str, struct io_internal_read_struct *iis)
+{
+    return (long)rb_str_locktmp_ensure(str, read_internal_call, (VALUE)iis);
 }
 
 static int
@@ -2765,7 +2781,7 @@ io_getpartial(int argc, VALUE *argv, VALUE io, VALUE opts, int nonblock)
     rb_io_t *fptr;
     VALUE length, str;
     long n, len;
-    struct read_internal_arg arg;
+    struct io_internal_read_struct iis;
     int shrinkable;
 
     rb_scan_args(argc, argv, "11", &length, &str);
@@ -2792,11 +2808,11 @@ io_getpartial(int argc, VALUE *argv, VALUE io, VALUE opts, int nonblock)
             rb_io_set_nonblock(fptr);
         }
 	io_setstrbuf(&str, len);
-	arg.fd = fptr->fd;
-	arg.str_ptr = RSTRING_PTR(str);
-	arg.len = len;
-	rb_str_locktmp_ensure(str, read_internal_call, (VALUE)&arg);
-	n = arg.len;
+        iis.fd = fptr->fd;
+        iis.nonblock = nonblock;
+        iis.buf = RSTRING_PTR(str);
+        iis.capa = len;
+        n = read_internal_locktmp(str, &iis);
         if (n < 0) {
 	    int e = errno;
             if (!nonblock && fptr_wait_readable(fptr))
@@ -2906,7 +2922,7 @@ io_read_nonblock(VALUE io, VALUE length, VALUE str, VALUE ex)
 {
     rb_io_t *fptr;
     long n, len;
-    struct read_internal_arg arg;
+    struct io_internal_read_struct iis;
     int shrinkable;
 
     if ((len = NUM2LONG(length)) < 0) {
@@ -2925,11 +2941,11 @@ io_read_nonblock(VALUE io, VALUE length, VALUE str, VALUE ex)
     if (n <= 0) {
 	rb_io_set_nonblock(fptr);
 	shrinkable |= io_setstrbuf(&str, len);
-	arg.fd = fptr->fd;
-	arg.str_ptr = RSTRING_PTR(str);
-	arg.len = len;
-	rb_str_locktmp_ensure(str, read_internal_call, (VALUE)&arg);
-	n = arg.len;
+        iis.fd = fptr->fd;
+        iis.nonblock = 1;
+        iis.buf = RSTRING_PTR(str);
+        iis.capa = len;
+        n = read_internal_locktmp(str, &iis);
         if (n < 0) {
 	    int e = errno;
 	    if ((e == EWOULDBLOCK || e == EAGAIN)) {
@@ -5094,7 +5110,7 @@ rb_io_sysread(int argc, VALUE *argv, VALUE io)
     VALUE len, str;
     rb_io_t *fptr;
     long n, ilen;
-    struct read_internal_arg arg;
+    struct io_internal_read_struct iis;
     int shrinkable;
 
     rb_scan_args(argc, argv, "11", &len, &str);
@@ -5122,12 +5138,11 @@ rb_io_sysread(int argc, VALUE *argv, VALUE io)
     rb_io_check_closed(fptr);
 
     io_setstrbuf(&str, ilen);
-    rb_str_locktmp(str);
-    arg.fd = fptr->fd;
-    arg.str_ptr = RSTRING_PTR(str);
-    arg.len = ilen;
-    rb_ensure(read_internal_call, (VALUE)&arg, rb_str_unlocktmp, str);
-    n = arg.len;
+    iis.fd = fptr->fd;
+    iis.nonblock = 1; /* for historical reasons, maybe (see above) */
+    iis.buf = RSTRING_PTR(str);
+    iis.capa = ilen;
+    n = read_internal_locktmp(str, &iis);
 
     if (n == -1) {
 	rb_sys_fail_path(fptr->pathv);
diff --git a/test/ruby/test_io.rb b/test/ruby/test_io.rb
index 9d0e5bc411..a933c93866 100644
--- a/test/ruby/test_io.rb
+++ b/test/ruby/test_io.rb
@@ -1360,7 +1360,6 @@ class TestIO < Test::Unit::TestCase
   def test_readpartial_lock
     with_pipe do |r, w|
       s = ""
-      r.nonblock = false if have_nonblock?
       t = Thread.new { r.readpartial(5, s) }
       Thread.pass until t.stop?
       assert_raise(RuntimeError) { s.clear }
@@ -3256,17 +3255,12 @@ __END__
 
       assert_equal 100, buf.bytesize
 
-      begin
+      msg = /can't modify string; temporarily locked/
+      assert_raise_with_message(RuntimeError, msg) do
         buf.replace("")
-      rescue RuntimeError => e
-        assert_match(/can't modify string; temporarily locked/, e.message)
-        Thread.pass
-      end until buf.empty?
-
-      assert_empty(buf, bug6099)
+      end
       assert_predicate(th, :alive?)
       w.write(data)
-      Thread.pass while th.alive?
       th.join
     end
     assert_equal(data, buf, bug6099)