jruby · Nov 9, 2014 · Nov 9, 2014
Showing with 51 additions and 44 deletions.

+36 −20 core/src/main/java/org/jruby/RubyIO.java

+15 −24 core/src/main/java/org/jruby/util/io/OpenFile.java
diff --git a/core/src/main/java/org/jruby/RubyIO.java b/core/src/main/java/org/jruby/RubyIO.java
@@ -407,24 +407,37 @@ protected RubyIO reopenIO(ThreadContext context, RubyIO nfile) {
                 throw runtime.newArgumentError(fptr.PREP_STDIO_NAME() + " can't change access mode from \"" + fptr.getModeAsString(runtime) + "\" to \"" + orig.getModeAsString(runtime) + "\"");
             }
         }
-        if (fptr.isWritable()) {
-            if (fptr.io_fflush(context) < 0)
-                throw runtime.newErrnoFromErrno(fptr.errno(), fptr.getPath());
-        }
-        else {
-            fptr.tell(context);
-        }
-        if (orig.isReadable()) {
-            pos = orig.tell(context);
+        // FIXME: three lock acquires...trying to reduce risk of deadlock, but not sure it's possible.
+
+        boolean locked = fptr.lock();
+        try {
+            if (fptr.isWritable()) {
+                if (fptr.io_fflush(context) < 0)
+                    throw runtime.newErrnoFromErrno(fptr.errno(), fptr.getPath());
+            } else {
+                fptr.tell(context);
+            }
+        } finally {
+            if (locked) fptr.unlock();
         }
-        if (orig.isWritable()) {
-            if (orig.io_fflush(context) < 0)
-                throw runtime.newErrnoFromErrno(orig.errno(), fptr.getPath());
+
+        locked = orig.lock();
+        try {
+            if (orig.isReadable()) {
+                pos = orig.tell(context);
+            }
+            if (orig.isWritable()) {
+                if (orig.io_fflush(context) < 0)
+                    throw runtime.newErrnoFromErrno(orig.errno(), fptr.getPath());
+            }
+        } finally {
+            if (locked) orig.unlock();
         }
 
         /* copy rb_io_t structure */
         // NOTE: MRI does not copy sync here, but I can find no way to make stdout/stderr stay sync through a reopen
-        boolean locked = fptr.lock();
+        locked = fptr.lock();
+        boolean locked2 = orig.lock(); // TODO: This WILL deadlock if two threads try to reopen the same IOs in opposite directions. Fix?
         try {
             fptr.setMode(orig.getMode() | (fptr.getMode() & (OpenFile.PREP | OpenFile.SYNC)));
             fptr.setProcess(orig.getProcess());
@@ -476,6 +489,7 @@ protected RubyIO reopenIO(ThreadContext context, RubyIO nfile) {
                 setBinmode();
             }
         } finally {
+            if (locked2) orig.unlock();
             if (locked) fptr.unlock();
         }
 
@@ -1476,11 +1490,11 @@ public RubyBoolean sync(ThreadContext context) {
 
         RubyIO io = GetWriteIO();
         fptr = io.getOpenFileChecked();
-        fptr.lockReadOnly();
+        fptr.lock();
         try {
             return (fptr.getMode() & OpenFile.SYNC) != 0 ? runtime.getTrue() : runtime.getFalse();
         } finally {
-            fptr.unlockReadOnly();
+            fptr.unlock();
         }
     }
 
@@ -1806,13 +1820,13 @@ public RubyBoolean tty_p(ThreadContext context) {
 
         fptr = getOpenFileChecked();
 
-        fptr.lockReadOnly();
+        fptr.lock();
         try {
             if (fptr.isStdio()) return runtime.getTrue();
             if (runtime.getPosix().isNative() && runtime.getPosix().libc().isatty(fptr.getFileno()) != 0)
                 return runtime.getTrue();
         } finally {
-            fptr.unlockReadOnly();
+            fptr.unlock();
         }
 
         return runtime.getFalse();
@@ -1836,8 +1850,9 @@ public IRubyObject initialize_copy(IRubyObject _io){
         orig = io.getOpenFileChecked();
         fptr = dest.MakeOpenFile();
 
-        // orig is the visible one here
-        orig.lockReadOnly();
+        // orig is the visible one here but we lock both anyway
+        boolean locked1 = orig.lock();
+        boolean locked2 = fptr.lock();
         try {
             io.flush(context);
 
@@ -1860,7 +1875,8 @@ public IRubyObject initialize_copy(IRubyObject _io){
             if (0 <= pos)
                 fptr.seek(context, pos, PosixShim.SEEK_SET);
         } finally {
-            orig.unlockReadOnly();
+            if (locked2) fptr.unlock();
+            if (locked1) orig.unlock();
         }
 
         if (fptr.isBinmode()) {

diff --git a/core/src/main/java/org/jruby/util/io/OpenFile.java b/core/src/main/java/org/jruby/util/io/OpenFile.java
@@ -41,6 +41,7 @@
 import java.nio.channels.WritableByteChannel;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 public class OpenFile implements Finalizable {
@@ -130,9 +131,7 @@ public static class Buffer {
     public boolean writeconvInitialized;
 
     public volatile ReentrantReadWriteLock write_lock;
-    private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
-    private final ReentrantReadWriteLock.WriteLock writeLock = lock.writeLock();
-    private final ReentrantReadWriteLock.ReadLock readLock = lock.readLock();
+    private final ReentrantLock lock = new ReentrantLock();
 
     public final Buffer wbuf = new Buffer(), rbuf = new Buffer(), cbuf = new Buffer();
 
@@ -410,7 +409,7 @@ public void checkReadable(ThreadContext context) {
 
     // io_fflush
     public int io_fflush(ThreadContext context) {
-        assert writeLock.isHeldByCurrentThread();
+        assert lock.isHeldByCurrentThread();
 
         checkClosed();
 
@@ -430,7 +429,7 @@ public int io_fflush(ThreadContext context) {
 
     // rb_io_wait_writable
     public boolean waitWritable(ThreadContext context, long timeout) {
-        assert writeLock.isHeldByCurrentThread();
+        assert lock.isHeldByCurrentThread();
 
         if (posix.errno == null) return false;
 
@@ -457,7 +456,7 @@ public boolean waitWritable(ThreadContext context) {
 
     // rb_io_wait_readable
     public boolean waitReadable(ThreadContext context, long timeout) {
-        assert writeLock.isHeldByCurrentThread();
+        assert lock.isHeldByCurrentThread();
 
         if (posix.errno == null) return false;
 
@@ -495,7 +494,7 @@ public boolean ready(ThreadContext context, int ops) {
      * @return
      */
     public boolean ready(Ruby runtime, RubyThread thread, int ops, long timeout) {
-        assert writeLock.isHeldByCurrentThread();
+        assert lock.isHeldByCurrentThread();
 
         try {
             if (fd.chSelect != null) {
@@ -637,15 +636,15 @@ boolean wsplit()
 
     // io_seek
     public long seek(ThreadContext context, long offset, int whence) {
-        assert writeLock.isHeldByCurrentThread();
+        assert lock.isHeldByCurrentThread();
 
         flushBeforeSeek(context);
         return posix.lseek(fd, offset, whence);
     }
 
     // flush_before_seek
     private void flushBeforeSeek(ThreadContext context) {
-        assert writeLock.isHeldByCurrentThread();
+        assert lock.isHeldByCurrentThread();
 
         if (io_fflush(context) < 0)
             throw context.runtime.newErrnoFromErrno(posix.errno, "");
@@ -2393,12 +2392,12 @@ public boolean isStdio() {
     }
 
     public int readPending() {
-        lockReadOnly();
+        lock();
         try {
             if (READ_CHAR_PENDING()) return 1;
             return READ_DATA_PENDING_COUNT();
         } finally {
-            unlockReadOnly();
+            unlock();
         }
     }
 
@@ -2618,30 +2617,22 @@ public int remainSize() {
         return siz;
     }
 
-    public void lockReadOnly() {
-        readLock.lock();
-    }
-
-    public void unlockReadOnly() {
-        readLock.unlock();
-    }
-
     public boolean lock() {
-        if (writeLock.isHeldByCurrentThread()) {
+        if (lock.isHeldByCurrentThread()) {
             return false;
         } else {
-            writeLock.lock();
+            lock.lock();
             return true;
         }
     }
 
     public void unlock() {
-        assert writeLock.isHeldByCurrentThread();
+        assert lock.isHeldByCurrentThread();
 
-        writeLock.unlock();
+        lock.unlock();
     }
 
     public boolean lockedByMe() {
-        return writeLock.isHeldByCurrentThread();
+        return lock.isHeldByCurrentThread();
     }
 }