Fix up non-native popen init and closing logic wrt write streams.

MRI uses a hidden attribute on IO objects made up of two separate read and write streams (tied_io_for_writing), when it must wrap those streams as if they're a single IO. This is the case for popen, and so we put the write stream into this "tied" field. However, we're only supposed to use the tied field when there's already a read stream. When there's just a write stream, as in The other part of this patch fixes the close logic for non-native popen, so that it will check if either the main stream or the "tied" stream is open before deciding whether to close it. The old logic only considered the primary stream. The first part of this patch (not using "tied" when only writing) hides the failure fixed by the second part of this patch since we see a single-stream normal IO and the original bug does not happen. Note that this probably regressed in 9k because we started using this "tied" field most places, but the non-native popen bits were not updated to test it. If we work on getting more tests green on Windows we should be able to find and eliminate these discrepancies. Fixes #3473.
jruby · Mar 14, 2016 · 16aef81 · 16aef81
1 parent 5837373
commit 16aef81
Showing 1 changed file with 13 additions and 15 deletions.
diff --git a/core/src/main/java/org/jruby/RubyIO.java b/core/src/main/java/org/jruby/RubyIO.java
@@ -1952,7 +1952,6 @@ public IRubyObject close() {
         if (isClosed()) {
             return runtime.getNil();
         }
-        openFile.checkClosed();
         return rbIoClose(runtime);
     }
 
@@ -3734,7 +3733,6 @@ private void setupPopen(ModeFlags modes, POpenProcess process) throws RaiseExcep
             ChannelFD main = new ChannelFD(inChannel, runtime.getPosix(), runtime.getFilenoUtil());
 
             openFile.setFD(main);
-            openFile.setMode(OpenFile.READABLE);
         }
 
         if (openFile.isWritable() && process.hasOutput()) {
@@ -3748,11 +3746,16 @@ private void setupPopen(ModeFlags modes, POpenProcess process) throws RaiseExcep
 
             ChannelFD pipe = new ChannelFD(outChannel, runtime.getPosix(), runtime.getFilenoUtil());
 
-            RubyIO writeIO = new RubyIO(runtime, runtime.getIO());
-            writeIO.initializeCommon(runtime.getCurrentContext(), pipe, runtime.newFixnum(OpenFlags.O_WRONLY), runtime.getNil());
+            // if also readable, attach as tied IO; otherwise, primary IO
+            if (openFile.isReadable()) {
+                RubyIO writeIO = new RubyIO(runtime, runtime.getIO());
+                writeIO.initializeCommon(runtime.getCurrentContext(), pipe, runtime.newFixnum(OpenFlags.O_WRONLY), runtime.getNil());
 
-            openFile.tiedIOForWriting = writeIO;
-            setInstanceVariable("@tied_io_for_writing", writeIO);
+                openFile.tiedIOForWriting = writeIO;
+                setInstanceVariable("@tied_io_for_writing", writeIO);
+            } else {
+                openFile.setFD(pipe);
+            }
         }
     }
 
@@ -3884,15 +3887,10 @@ public static IRubyObject popen(ThreadContext context, IRubyObject recv, IRubyOb
             io.setupPopen(modes, process);
 
             if (block.isGiven()) {
-                try {
-                    return block.yield(context, io);
-                } finally {
-                    if (io.openFile.isOpen()) {
-                        io.close();
-                    }
-                    // RubyStatus uses real native status now, so we unshift Java's shifted exit status
-                    context.setLastExitStatus(RubyProcess.RubyStatus.newProcessStatus(runtime, process.waitFor() << 8, ShellLauncher.getPidFromProcess(process)));
-                }
+                ensureYieldClose(context, io, block);
+
+                // RubyStatus uses real native status now, so we unshift Java's shifted exit status
+                context.setLastExitStatus(RubyProcess.RubyStatus.newProcessStatus(runtime, process.waitFor() << 8, ShellLauncher.getPidFromProcess(process)));
             }
             return io;
         } catch (IOException e) {