Avoid deadlocks in IO by only locking one thing. Fixes #4854

There are two changes here, with the first the more likely cause of #4854: 1. While leaving the OpenFile lock locked, we proceeded to attempt to lock the Selector being selected. If the order of these locks happens differently elsewhere, it will cause a deadlock. Fixed by unlocking the OpenFile lock before locking the Selector lock. 2. OpenFile.removeBlockingThread synchronized on both OpenFile's lock and the OpenFile instance itself. I could find only a handful of other places that lock OpenFile and they do not appear to lock anything else, but there's no reason to lock OpenFile here anyway.
jruby · Dec 5, 2017 · fd1f47a · fd1f47a
1 parent a8bfd60
commit fd1f47a
Showing 2 changed files with 71 additions and 57 deletions.
diff --git a/core/src/main/java/org/jruby/RubyThread.java b/core/src/main/java/org/jruby/RubyThread.java
@@ -1753,72 +1753,86 @@ public boolean select(Channel channel, OpenFile fptr, int ops, long timeout) {
         if (channel instanceof SelectableChannel && (fptr == null || !fptr.fd().isNativeFile)) {
             SelectableChannel selectable = (SelectableChannel)channel;
 
-            synchronized (selectable.blockingLock()) {
-                boolean oldBlocking = selectable.isBlocking();
-
-                SelectionKey key;
-                try {
-                    selectable.configureBlocking(false);
-
-                    if (fptr != null) fptr.addBlockingThread(this);
-                    currentSelector = getRuntime().getSelectorPool().get(selectable.provider());
-
-                    key = selectable.register(currentSelector, ops);
-
-                    beforeBlockingCall();
-                    int result;
-                    if (timeout < 0) {
-                        result = currentSelector.select();
-                    } else if (timeout == 0) {
-                        result = currentSelector.selectNow();
-                    } else {
-                        result = currentSelector.select(timeout);
-                    }
+            // ensure we have fptr locked, but release it to avoid deadlock
+            boolean locked = false;
+            if (fptr != null) {
+                locked = fptr.lock();
+                fptr.unlock();
+            }
+            try {
+                synchronized (selectable.blockingLock()) {
+                    boolean oldBlocking = selectable.isBlocking();
+
+                    SelectionKey key;
+                    try {
+                        selectable.configureBlocking(false);
 
-                    // check for thread events, in case we've been woken up to die
-                    pollThreadEvents();
+                        if (fptr != null) fptr.addBlockingThread(this);
+                        currentSelector = getRuntime().getSelectorPool().get(selectable.provider());
 
-                    if (result == 1) {
-                        Set<SelectionKey> keySet = currentSelector.selectedKeys();
+                        key = selectable.register(currentSelector, ops);
 
-                        if (keySet.iterator().next() == key) {
-                            return true;
+                        beforeBlockingCall();
+                        int result;
+
+                        if (timeout < 0) {
+                            result = currentSelector.select();
+                        } else if (timeout == 0) {
+                            result = currentSelector.selectNow();
+                        } else {
+                            result = currentSelector.select(timeout);
                         }
-                    }
 
-                    return false;
-                } catch (IOException ioe) {
-                    throw getRuntime().newIOErrorFromException(ioe);
-                } finally {
-                    // Note: I don't like ignoring these exceptions, but it's
-                    // unclear how likely they are to happen or what damage we
-                    // might do by ignoring them. Note that the pieces are separate
-                    // so that we can ensure one failing does not affect the others
-                    // running.
-
-                    // shut down and null out the selector
-                    try {
-                        if (currentSelector != null) {
-                            getRuntime().getSelectorPool().put(currentSelector);
+                        // check for thread events, in case we've been woken up to die
+                        pollThreadEvents();
+
+                        if (result == 1) {
+                            Set<SelectionKey> keySet = currentSelector.selectedKeys();
+
+                            if (keySet.iterator().next() == key) {
+                                return true;
+                            }
                         }
-                    } catch (Exception e) {
-                        // ignore
+
+                        return false;
+                    } catch (IOException ioe) {
+                        throw getRuntime().newIOErrorFromException(ioe);
                     } finally {
-                        currentSelector = null;
-                    }
+                        // Note: I don't like ignoring these exceptions, but it's
+                        // unclear how likely they are to happen or what damage we
+                        // might do by ignoring them. Note that the pieces are separate
+                        // so that we can ensure one failing does not affect the others
+                        // running.
+
+                        // shut down and null out the selector
+                        try {
+                            if (currentSelector != null) {
+                                getRuntime().getSelectorPool().put(currentSelector);
+                            }
+                        } catch (Exception e) {
+                            // ignore
+                        } finally {
+                            currentSelector = null;
+                        }
 
-                    // remove this thread as a blocker against the given IO
-                    if (fptr != null) fptr.removeBlockingThread(this);
+                        // remove this thread as a blocker against the given IO
+                        if (fptr != null) fptr.removeBlockingThread(this);
 
-                    // go back to previous blocking state on the selectable
-                    try {
-                        selectable.configureBlocking(oldBlocking);
-                    } catch (Exception e) {
-                        // ignore
-                    }
+                        // go back to previous blocking state on the selectable
+                        try {
+                            selectable.configureBlocking(oldBlocking);
+                        } catch (Exception e) {
+                            // ignore
+                        }
 
-                    // clear thread state from blocking call
-                    afterBlockingCall();
+                        // clear thread state from blocking call
+                        afterBlockingCall();
+                    }
+                }
+            } finally {
+                if (fptr != null) {
+                    fptr.lock();
+                    if (locked) fptr.unlock();
                 }
             }
         } else {

diff --git a/core/src/main/java/org/jruby/util/io/OpenFile.java b/core/src/main/java/org/jruby/util/io/OpenFile.java
@@ -2625,7 +2625,7 @@ public void addBlockingThread(RubyThread thread) {
      *
      * @param thread A thread blocking on this IO
      */
-    public synchronized void removeBlockingThread(RubyThread thread) {
+    public void removeBlockingThread(RubyThread thread) {
         boolean locked = lock();
         try {
             if (blockingThreads == null) {