[FIXED JENKINS-24050] don't let canceled keys kill the selector thread

In looking at the proposed PR #24 (#24), I feel bit uneasy to mask the problem like it does. The code in question is looping through selected keys and processing it one by one. The only code that calls key.cancel() is done from the selector thread that runs this loop. So I don't understand how it is possible that the key picked up from selected key set is already cancelled here. I wonder if something more is going on. Regardless, I agree that this shouldn't kill the selector thread, which breaks all the slaves in one go. This change flags and reports the problem, kill the connection related to that key, then continue to serve other connections.
jenkinsci · Sep 5, 2014 · 1083a97 · 1083a97 · kbrowder · Sep 6, 2014
1 parent 602dadc
commit 1083a97
Showing 1 changed file with 13 additions and 0 deletions.
diff --git a/src/main/java/org/jenkinsci/remoting/nio/NioChannelHub.java b/src/main/java/org/jenkinsci/remoting/nio/NioChannelHub.java
@@ -17,6 +17,7 @@
 import java.io.InputStream;
 import java.io.InterruptedIOException;
 import java.io.OutputStream;
+import java.nio.channels.CancelledKeyException;
 import java.nio.channels.ClosedSelectorException;
 import java.nio.channels.ReadableByteChannel;
 import java.nio.channels.SelectableChannel;
@@ -325,6 +326,9 @@ public void reregister() throws IOException {
             }
         }
 
+        /**
+         * If both directions are closed, cancel the whole key.
+         */
         @SelectorThreadOnly
         private void maybeCancelKey() throws IOException {
             SelectionKey key = ch.keyFor(selector);
@@ -579,6 +583,15 @@ public void run() {
                         } catch (IOException e) {
                             LOGGER.log(WARNING, "Communication problem", e);
                             t.abort(e);
+                        } catch (CancelledKeyException e) {
+                            // see JENKINS-24050. I don't understand how this can happen, given that the selector
+                            // thread is the only thread that cancels keys. So to better understand what's going on,
+                            // report the problem.
+                            LOGGER.log(SEVERE, "Unexpected key cancellation for "+t, e);
+                            // to be on the safe side, abort the communication. if we don't do this, it's possible
+                            // that the key never gets re-registered to the selector, and the traffic will hang
+                            // on this channel.
+                            t.abort(e);
                         }
                     } else {
                         onSelected(key);