8231603: (se) Selector implementations do not need to use cancelledKeys

Reviewed-by: chegar, bpb

src/java.base/share/classes/java/nio/channels/spi/AbstractSelectionKey.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,8 +25,13 @@
 
 package java.nio.channels.spi;
 
-import java.nio.channels.*;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.VarHandle;
+import java.nio.channels.SelectionKey;
+import java.nio.channels.Selector;
 
+import sun.nio.ch.SelectionKeyImpl;
+import sun.nio.ch.SelectorImpl;
 
 /**
  * Base implementation class for selection keys.
@@ -41,20 +46,29 @@ import java.nio.channels.*;
 public abstract class AbstractSelectionKey
     extends SelectionKey
 {
+    private static final VarHandle INVALID;
+    static {
+        try {
+            MethodHandles.Lookup l = MethodHandles.lookup();
+            INVALID = l.findVarHandle(AbstractSelectionKey.class, "invalid", boolean.class);
+        } catch (Exception e) {
+            throw new InternalError(e);
+        }
+    }
 
     /**
      * Initializes a new instance of this class.
      */
     protected AbstractSelectionKey() { }
 
-    private volatile boolean valid = true;
+    private volatile boolean invalid;
 
     public final boolean isValid() {
-        return valid;
+        return !invalid;
     }
 
     void invalidate() {                                 // package-private
-        valid = false;
+        invalid = true;
     }
 
     /**
@@ -64,13 +78,14 @@ public abstract class AbstractSelectionKey
      * selector's cancelled-key set while synchronized on that set.  </p>
      */
     public final void cancel() {
-        // Synchronizing "this" to prevent this key from getting canceled
+        boolean changed = (boolean) INVALID.compareAndSet(this, false, true);
-        // multiple times by different threads, which might cause race
+        if (changed) {
-        // condition between selector's select() and channel's close().
+            Selector sel = selector();
-        synchronized (this) {
+            if (sel instanceof SelectorImpl) {
-            if (valid) {
+                // queue cancelled key directly
-                valid = false;
+                ((SelectorImpl) sel).cancel((SelectionKeyImpl) this);
-                ((AbstractSelector)selector()).cancel(this);
+            } else {
+                ((AbstractSelector) sel).cancel(this);
             }
         }
     }

src/java.base/share/classes/java/nio/channels/spi/AbstractSelector.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,12 +26,14 @@
 package java.nio.channels.spi;
 
 import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.VarHandle;
 import java.nio.channels.SelectionKey;
 import java.nio.channels.Selector;
 import java.util.HashSet;
 import java.util.Set;
 import sun.nio.ch.Interruptible;
-import java.util.concurrent.atomic.AtomicBoolean;
+import sun.nio.ch.SelectorImpl;
 
 
 /**
@@ -69,12 +71,23 @@ import java.util.concurrent.atomic.AtomicBoolean;
 public abstract class AbstractSelector
     extends Selector
 {
-
+    private static final VarHandle CLOSED;
-    private final AtomicBoolean selectorOpen = new AtomicBoolean(true);
+    static {
+        try {
+            MethodHandles.Lookup l = MethodHandles.lookup();
+            CLOSED = l.findVarHandle(AbstractSelector.class, "closed", boolean.class);
+        } catch (Exception e) {
+            throw new InternalError(e);
+        }
+    }
+    private volatile boolean closed;
 
     // The provider that created this selector
     private final SelectorProvider provider;
 
+    // cancelled-key, not used by the JDK Selector implementations
+    private final Set<SelectionKey> cancelledKeys;
+
     /**
      * Initializes a new instance of this class.
      *
@@ -83,10 +96,14 @@ public abstract class AbstractSelector
      */
     protected AbstractSelector(SelectorProvider provider) {
         this.provider = provider;
+        if (this instanceof SelectorImpl) {
+            // not used in JDK Selector implementations
+            this.cancelledKeys = Set.of();
+        } else {
+            this.cancelledKeys = new HashSet<>();
+        }
     }
 
-    private final Set<SelectionKey> cancelledKeys = new HashSet<SelectionKey>();
-
     void cancel(SelectionKey k) {                       // package-private
         synchronized (cancelledKeys) {
             cancelledKeys.add(k);
@@ -105,10 +122,10 @@ public abstract class AbstractSelector
      *          If an I/O error occurs
      */
     public final void close() throws IOException {
-        boolean open = selectorOpen.getAndSet(false);
+        boolean changed = (boolean) CLOSED.compareAndSet(this, false, true);
-        if (!open)
+        if (changed) {
-            return;
+            implCloseSelector();
-        implCloseSelector();
+        }
     }
 
     /**
@@ -130,7 +147,7 @@ public abstract class AbstractSelector
     protected abstract void implCloseSelector() throws IOException;
 
     public final boolean isOpen() {
-        return selectorOpen.get();
+        return !closed;
     }
 
     /**

src/java.base/share/classes/sun/nio/ch/SelectorImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -33,7 +33,9 @@ import java.nio.channels.SelectionKey;
 import java.nio.channels.spi.AbstractSelectableChannel;
 import java.nio.channels.spi.AbstractSelector;
 import java.nio.channels.spi.SelectorProvider;
+import java.util.ArrayDeque;
 import java.util.Collections;
+import java.util.Deque;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Objects;
@@ -46,7 +48,7 @@ import java.util.function.Consumer;
  * Base Selector implementation class.
  */
 
-abstract class SelectorImpl
+public abstract class SelectorImpl
     extends AbstractSelector
 {
     // The set of keys registered with this Selector
@@ -59,6 +61,9 @@ abstract class SelectorImpl
     private final Set<SelectionKey> publicKeys;             // Immutable
     private final Set<SelectionKey> publicSelectedKeys;     // Removal allowed, but not addition
 
+    // pending cancelled keys for deregistration
+    private final Deque<SelectionKeyImpl> cancelledKeys = new ArrayDeque<>();
+
     // used to check for reentrancy
     private boolean inSelect;
 
@@ -239,33 +244,36 @@ abstract class SelectorImpl
     protected abstract void implDereg(SelectionKeyImpl ski) throws IOException;
 
     /**
-     * Invoked by selection operations to process the cancelled-key set
+     * Queue a cancelled key for the next selection operation
+     */
+    public void cancel(SelectionKeyImpl ski) {
+        synchronized (cancelledKeys) {
+            cancelledKeys.addLast(ski);
+        }
+    }
+
+    /**
+     * Invoked by selection operations to process the cancelled keys
      */
     protected final void processDeregisterQueue() throws IOException {
         assert Thread.holdsLock(this);
         assert Thread.holdsLock(publicSelectedKeys);
 
-        Set<SelectionKey> cks = cancelledKeys();
+        synchronized (cancelledKeys) {
-        synchronized (cks) {
+            SelectionKeyImpl ski;
-            if (!cks.isEmpty()) {
+            while ((ski = cancelledKeys.pollFirst()) != null) {
-                Iterator<SelectionKey> i = cks.iterator();
+                // remove the key from the selector
-                while (i.hasNext()) {
+                implDereg(ski);
-                    SelectionKeyImpl ski = (SelectionKeyImpl)i.next();
-                    i.remove();
 
-                    // remove the key from the selector
+                selectedKeys.remove(ski);
-                    implDereg(ski);
+                keys.remove(ski);
 
-                    selectedKeys.remove(ski);
+                // remove from channel's key set
-                    keys.remove(ski);
+                deregister(ski);
 
-                    // remove from channel's key set
+                SelectableChannel ch = ski.channel();
-                    deregister(ski);
+                if (!ch.isOpen() && !ch.isRegistered())
-
+                    ((SelChImpl)ch).kill();
-                    SelectableChannel ch = ski.channel();
-                    if (!ch.isOpen() && !ch.isRegistered())
-                        ((SelChImpl)ch).kill();
-                }
             }
         }
     }