[ruby-dev:32205] Use two pipes for duplex IO.popen

改心して duplex な IO.popen で socketpair を使うのはやめよう
と思うのですが、どうでしょうか。

というのは、起動したプログラムが標準出力をクローズしたことを
親が検知できないからです。

% cat close-stdout.c 
#include <unistd.h>

int main(int argc, char **argv)
{
  close(1);
  sleep(2);
  return 0;
}
% gcc -o close-stdout close-stdout.c

というような、標準出力を即座にクローズし、その後で 2秒寝る、
というプログラムがあったとします。

これは 1.8 では次のように即座に終わります。

% time ruby-1.8 -ve 'p IO.popen("./close-stdout", "r+").read'        
ruby 1.8.6 (2007-11-09 patchlevel 5000) [i686-linux]
""
ruby-1.8 -ve 'p IO.popen("./close-stdout", "r+").read' 0.00s user 0.00s system 71% cpu 0.006 total

しかし 1.9 では 2秒かかります。

% time ./ruby -ve 'p IO.popen("./close-stdout", "r+").read'  
ruby 1.9.0 (2007-11-10 patchlevel 0) [i686-linux]
""
./ruby -ve 'p IO.popen("./close-stdout", "r+").read'  0.01s user 0.00s system 0% cpu 2.020 total

これは socketpair で EOF を伝えるためには half close しない
といけなくて、そのためには shutdown する必要があるけれど起動
された側のプログラムはそんなことは知ったことでは無い、という
のが問題です。

これは socketpair を使う限りは解決できないので、1.8 のように
パイプを 2本使って、普通に close すれば親がそれを検知できる
のが適切であるという結論に至りました。

なお、IO ひとつに 2つの fd が入っていて fcntl 等の fd の操作
に支障を来すという問題は、書き込み側の fd にも独立した IO オ
ブジェクトを作り、それを @write_io というインスタンス変数で
参照可能にすることにより、操作不可能ではないようにしてありま
す。

Index: include/ruby/intern.h
===================================================================
--- include/ruby/intern.h	(リビジョン 13873)
+++ include/ruby/intern.h	(作業コピー)
@@ -353,6 +353,7 @@
 VALUE rb_io_print(int, VALUE*, VALUE);
 VALUE rb_io_puts(int, VALUE*, VALUE);
 VALUE rb_io_fdopen(int, int, const char*);
+VALUE rb_io_get_write_io(VALUE);
 VALUE rb_file_open(const char*, const char*);
 VALUE rb_gets(void);
 void rb_write_error(const char*);
Index: include/ruby/io.h
===================================================================
--- include/ruby/io.h	(リビジョン 13873)
+++ include/ruby/io.h	(作業コピー)
@@ -45,6 +45,7 @@
     int rbuf_off;
     int rbuf_len;
     int rbuf_capa;
+    int has_write_io;
     rb_encoding *enc;
 } rb_io_t;
 
@@ -88,8 +89,12 @@
     fp->rbuf_off = 0;\
     fp->rbuf_len = 0;\
     fp->rbuf_capa = 0;\
+    fp->has_write_io = 0;\
 } while (0)
 
+#define GetReadIO(io) (io)
+#define GetWriteIO(io) rb_io_get_write_io(io)
+
 FILE *rb_io_stdio_file(rb_io_t *fptr);
 
 FILE *rb_fopen(const char*, const char*);
Index: io.c
===================================================================
--- io.c	(リビジョン 13873)
+++ io.c	(作業コピー)
@@ -246,6 +246,21 @@
     return rb_check_convert_type(io, T_FILE, "IO", "to_io");
 }
 
+VALUE
+rb_io_get_write_io(VALUE io)
+{
+    VALUE write_io;
+    if (RFILE(io)->fptr->has_write_io) {
+        write_io = rb_ivar_get(io, rb_intern("@write_io"));
+        if (RTEST(write_io)) {
+            write_io = rb_io_get_io(write_io);
+            if (!RFILE(write_io)->fptr->has_write_io)
+                return write_io;
+        }
+    }
+    return io;
+}
+
 /*
  *  call-seq:
  *     IO.try_convert(obj) -> io or nil
@@ -676,6 +691,7 @@
     VALUE tmp;
 
     rb_secure(4);
+    io = GetWriteIO(io);
     str = rb_obj_as_string(str);
     tmp = rb_io_check_io(io);
     if (NIL_P(tmp)) {
@@ -748,6 +764,7 @@
         return rb_funcall(io, id_flush, 0);
     }
 
+    io = GetWriteIO(io);
     GetOpenFile(io, fptr);
 
     if (fptr->mode & FMODE_WRITABLE) {
@@ -982,6 +999,7 @@
 {
     rb_io_t *fptr;
 
+    io = GetWriteIO(io);
     GetOpenFile(io, fptr);
     return (fptr->mode & FMODE_SYNC) ? Qtrue : Qfalse;
 }
@@ -1006,6 +1024,7 @@
 {
     rb_io_t *fptr;
 
+    io = GetWriteIO(io);
     GetOpenFile(io, fptr);
     if (RTEST(mode)) {
 	fptr->mode |= FMODE_SYNC;
@@ -1034,6 +1053,7 @@
 #ifdef HAVE_FSYNC
     rb_io_t *fptr;
 
+    io = GetWriteIO(io);
     GetOpenFile(io, fptr);
 
     io_fflush(fptr);
@@ -1464,6 +1484,7 @@
     if (TYPE(str) != T_STRING)
 	str = rb_obj_as_string(str);
 
+    io = GetWriteIO(io);
     GetOpenFile(io, fptr);
     rb_io_check_writable(fptr);
 
@@ -2348,7 +2369,17 @@
 {
     rb_io_t *fptr;
     int fd;
+    VALUE write_io;
+    rb_io_t *write_fptr;
 
+    write_io = GetWriteIO(io);
+    if (io != write_io) {
+        write_fptr = RFILE(write_io)->fptr;
+        if (write_fptr && 0 <= write_fptr->fd) {
+            rb_io_fptr_cleanup(write_fptr, Qtrue);
+        }
+    }
+
     fptr = RFILE(io)->fptr;
     if (!fptr) return Qnil;
     if (fptr->fd < 0) return Qnil;
@@ -2425,7 +2456,17 @@
 rb_io_closed(VALUE io)
 {
     rb_io_t *fptr;
+    VALUE write_io;
+    rb_io_t *write_fptr;
 
+    write_io = GetWriteIO(io);
+    if (io != write_io) {
+        write_fptr = RFILE(write_io)->fptr;
+        if (write_fptr && 0 <= write_fptr->fd) {
+            return Qfalse;
+        }
+    }
+
     fptr = RFILE(io)->fptr;
     rb_io_check_initialized(fptr);
     return 0 <= fptr->fd ? Qfalse : Qtrue;
@@ -2453,6 +2494,7 @@
 rb_io_close_read(VALUE io)
 {
     rb_io_t *fptr;
+    VALUE write_io;
 
     if (rb_safe_level() >= 4 && !OBJ_TAINTED(io)) {
 	rb_raise(rb_eSecurityError, "Insecure: can't close");
@@ -2469,6 +2511,13 @@
             return rb_io_close(io);
         return Qnil;
     }
+
+    write_io = GetWriteIO(io);
+    if (io != write_io) {
+        fptr_finalize(fptr, Qfalse);
+        return Qnil;
+    }
+
     if (fptr->mode & FMODE_WRITABLE) {
 	rb_raise(rb_eIOError, "closing non-duplex IO for reading");
     }
@@ -2498,6 +2547,8 @@
 rb_io_close_write(VALUE io)
 {
     rb_io_t *fptr;
+    VALUE write_io;
+    rb_io_t *write_fptr;
 
     if (rb_safe_level() >= 4 && !OBJ_TAINTED(io)) {
 	rb_raise(rb_eSecurityError, "Insecure: can't close");
@@ -2515,6 +2566,13 @@
         return Qnil;
     }
 
+    write_io = GetWriteIO(io);
+    if (io != write_io) {
+        write_fptr = RFILE(write_io)->fptr;
+        fptr_finalize(write_fptr, Qfalse);
+        return Qnil;
+    }
+
     if (fptr->mode & FMODE_READABLE) {
 	rb_raise(rb_eIOError, "closing non-duplex IO for writing");
     }
@@ -2582,6 +2640,7 @@
     if (TYPE(str) != T_STRING)
 	str = rb_obj_as_string(str);
 
+    io = GetWriteIO(io);
     GetOpenFile(io, fptr);
     rb_io_check_writable(fptr);
 
@@ -2666,15 +2725,6 @@
     return str;
 }
 
-/*
- *  call-seq:
- *     ios.binmode    => ios
- *
- *  Puts <em>ios</em> into binary mode. This is useful only in
- *  MS-DOS/Windows environments. Once a stream is in binary mode, it
- *  cannot be reset to nonbinary mode.
- */
-
 VALUE
 rb_io_binmode(VALUE io)
 {
@@ -2693,6 +2743,30 @@
     return io;
 }
 
+/*
+ *  call-seq:
+ *     ios.binmode    => ios
+ *
+ *  Puts <em>ios</em> into binary mode. This is useful only in
+ *  MS-DOS/Windows environments. Once a stream is in binary mode, it
+ *  cannot be reset to nonbinary mode.
+ */
+
+static VALUE
+rb_io_binmode_m(VALUE io)
+{
+#if defined(_WIN32) || defined(DJGPP) || defined(__CYGWIN__) || defined(__human68k__) || defined(__EMX__)
+    VALUE write_io;
+
+    rb_io_binmode(io);
+
+    write_io = GetWriteIO(io);
+    if (write_io != io)
+        rb_io_binmode(write_io);
+#endif
+    return io;
+}
+
 static const char*
 rb_io_flags_mode(int flags)
 {
@@ -3078,17 +3152,23 @@
     struct rb_exec_arg exec;
     int modef;
     int pair[2];
+    int write_pair[2];
 };
 
 static void
 popen_redirect(struct popen_arg *p)
 {
     if ((p->modef & FMODE_READABLE) && (p->modef & FMODE_WRITABLE)) {
+        close(p->write_pair[1]);
+        if (p->write_pair[0] != 0) {
+            dup2(p->write_pair[0], 0);
+            close(p->write_pair[0]);
+        }
         close(p->pair[0]);
-        dup2(p->pair[1], 0);
-        dup2(p->pair[1], 1);
-        if (2 <= p->pair[1])
+        if (p->pair[1] != 1) {
+            dup2(p->pair[1], 1);
             close(p->pair[1]);
+        }
     }
     else if (p->modef & FMODE_READABLE) {
         close(p->pair[0]);
@@ -3132,6 +3212,8 @@
     int pid = 0;
     rb_io_t *fptr;
     VALUE port;
+    rb_io_t *write_fptr;
+    VALUE write_port;
 #if defined(HAVE_FORK)
     int status;
     struct popen_arg arg;
@@ -3141,17 +3223,24 @@
 #endif
     FILE *fp = 0;
     int fd = -1;
+    int write_fd = -1;
 
 #if defined(HAVE_FORK)
     arg.modef = modef;
     arg.pair[0] = arg.pair[1] = -1;
+    arg.write_pair[0] = arg.write_pair[1] = -1;
     switch (modef & (FMODE_READABLE|FMODE_WRITABLE)) {
-#if defined(HAVE_SOCKETPAIR)
       case FMODE_READABLE|FMODE_WRITABLE:
-        if (socketpair(AF_UNIX, SOCK_STREAM, 0, arg.pair) < 0)
+        if (pipe(arg.write_pair) < 0)
             rb_sys_fail(cmd);
+        if (pipe(arg.pair) < 0) {
+            int e = errno;
+            close(arg.write_pair[0]);
+            close(arg.write_pair[1]);
+            errno = e;
+            rb_sys_fail(cmd);
+        }
 	break;
-#endif
       case FMODE_READABLE:
         if (pipe(arg.pair) < 0)
             rb_sys_fail(cmd);
@@ -3187,12 +3276,18 @@
 	int e = errno;
 	close(arg.pair[0]);
 	close(arg.pair[1]);
+        if ((modef & (FMODE_READABLE|FMODE_WRITABLE)) == (FMODE_READABLE|FMODE_WRITABLE)) {
+            close(arg.write_pair[0]);
+            close(arg.write_pair[1]);
+        }
 	errno = e;
 	rb_sys_fail(cmd);
     }
     if ((modef & FMODE_READABLE) && (modef & FMODE_WRITABLE)) {
         close(arg.pair[1]);
         fd = arg.pair[0];
+        close(arg.write_pair[0]);
+        write_fd = arg.write_pair[1];
     }
     else if (modef & FMODE_READABLE) {
         close(arg.pair[1]);
@@ -3245,6 +3340,16 @@
     fptr->mode = modef | FMODE_SYNC|FMODE_DUPLEX;
     fptr->pid = pid;
 
+    if (0 <= write_fd) {
+        write_port = io_alloc(rb_cIO);
+        MakeOpenFile(write_port, write_fptr);
+        write_fptr->fd = write_fd;
+        write_fptr->mode = (modef & ~FMODE_READABLE)| FMODE_SYNC|FMODE_DUPLEX;
+        fptr->mode &= ~FMODE_WRITABLE;
+        rb_ivar_set(port, rb_intern("@write_io"), write_port);
+        fptr->has_write_io = 1;
+    }
+
 #if defined (__CYGWIN__) || !defined(HAVE_FORK)
     fptr->finalize = pipe_finalize;
     pipe_add_fptr(fptr);
@@ -3770,6 +3875,7 @@
 {
     rb_io_t *fptr, *orig;
     int fd;
+    VALUE write_io;
 
     io = rb_io_get_io(io);
     if (dest == io) return dest;
@@ -3792,6 +3898,13 @@
 	rb_io_binmode(dest);
     }
 
+    write_io = GetWriteIO(io);
+    if (io != write_io) {
+        write_io = rb_obj_dup(write_io);
+        rb_ivar_set(dest, rb_intern("@write_io"), write_io);
+        fptr->has_write_io = 1;
+    }
+
     return dest;
 }
 
@@ -4732,7 +4845,8 @@
     if (!NIL_P(write)) {
 	Check_Type(write, T_ARRAY);
 	for (i=0; i<RARRAY_LEN(write); i++) {
-	    GetOpenFile(rb_io_get_io(RARRAY_PTR(write)[i]), fptr);
+            VALUE write_io = GetWriteIO(rb_io_get_io(RARRAY_PTR(write)[i]));
+	    GetOpenFile(write_io, fptr);
 	    rb_fd_set(fptr->fd, &fds[1]);
 	    if (max < fptr->fd) max = fptr->fd;
 	}
@@ -4744,9 +4858,16 @@
     if (!NIL_P(except)) {
 	Check_Type(except, T_ARRAY);
 	for (i=0; i<RARRAY_LEN(except); i++) {
-	    GetOpenFile(rb_io_get_io(RARRAY_PTR(except)[i]), fptr);
+            VALUE io = rb_io_get_io(RARRAY_PTR(except)[i]);
+            VALUE write_io = GetWriteIO(io);
+	    GetOpenFile(io, fptr);
 	    rb_fd_set(fptr->fd, &fds[2]);
 	    if (max < fptr->fd) max = fptr->fd;
+            if (io != write_io) {
+                GetOpenFile(write_io, fptr);
+                rb_fd_set(fptr->fd, &fds[2]);
+                if (max < fptr->fd) max = fptr->fd;
+            }
 	}
 	ep = rb_fd_ptr(&fds[2]);
     }
@@ -4771,10 +4892,11 @@
 	if (rp) {
 	    list = RARRAY_PTR(res)[0];
 	    for (i=0; i< RARRAY_LEN(read); i++) {
-		GetOpenFile(rb_io_get_io(RARRAY_PTR(read)[i]), fptr);
+                VALUE io = rb_io_get_io(rb_ary_entry(read, i));
+		GetOpenFile(io, fptr);
 		if (rb_fd_isset(fptr->fd, &fds[0]) ||
 		    rb_fd_isset(fptr->fd, &fds[3])) {
-		    rb_ary_push(list, rb_ary_entry(read, i));
+		    rb_ary_push(list, io);
 		}
 	    }
 	}
@@ -4782,9 +4904,11 @@
 	if (wp) {
 	    list = RARRAY_PTR(res)[1];
 	    for (i=0; i< RARRAY_LEN(write); i++) {
-		GetOpenFile(rb_io_get_io(RARRAY_PTR(write)[i]), fptr);
+                VALUE io = rb_io_get_io(rb_ary_entry(write, i));
+                VALUE write_io = GetWriteIO(io);
+		GetOpenFile(write_io, fptr);
 		if (rb_fd_isset(fptr->fd, &fds[1])) {
-		    rb_ary_push(list, rb_ary_entry(write, i));
+		    rb_ary_push(list, io);
 		}
 	    }
 	}
@@ -4792,10 +4916,18 @@
 	if (ep) {
 	    list = RARRAY_PTR(res)[2];
 	    for (i=0; i< RARRAY_LEN(except); i++) {
-		GetOpenFile(rb_io_get_io(RARRAY_PTR(except)[i]), fptr);
+                VALUE io = rb_io_get_io(rb_ary_entry(write, i));
+                VALUE write_io = GetWriteIO(io);
+		GetOpenFile(io, fptr);
 		if (rb_fd_isset(fptr->fd, &fds[2])) {
-		    rb_ary_push(list, rb_ary_entry(except, i));
+		    rb_ary_push(list, io);
 		}
+                else if (io != write_io) {
+                    GetOpenFile(write_io, fptr);
+                    if (rb_fd_isset(fptr->fd, &fds[2])) {
+                        rb_ary_push(list, io);
+                    }
+                }
 	    }
 	}
     }
@@ -5906,7 +6038,7 @@
 
     rb_define_method(rb_cIO, "isatty", rb_io_isatty, 0);
     rb_define_method(rb_cIO, "tty?", rb_io_isatty, 0);
-    rb_define_method(rb_cIO, "binmode",  rb_io_binmode, 0);
+    rb_define_method(rb_cIO, "binmode",  rb_io_binmode_m, 0);
     rb_define_method(rb_cIO, "sysseek", rb_io_sysseek, -1);
 
     rb_define_method(rb_cIO, "ioctl", rb_io_ioctl, -1);

なお、Windows についてはいちおう binmode だけは処置したつも
りですが、うまくいっていないかもしれません。
-- 
[田中 哲][たなか あきら][Tanaka Akira]