5025260: Register methods should throw ClosedChannelException instead of NPE

Summary: update the spec and implementation to throw ClosedSelectorException
Reviewed-by: alanb

src/share/classes/java/nio/channels/SelectableChannel.java

@@ -191,6 +191,9 @@ public abstract class SelectableChannel
      * @throws  ClosedChannelException
      *          If this channel is closed
      *
+     * @throws  ClosedSelectorException
+     *          If the selector is closed
+     *
      * @throws  IllegalBlockingModeException
      *          If this channel is in blocking mode
      *
@@ -246,6 +249,9 @@ public abstract class SelectableChannel
      * @throws  ClosedChannelException
      *          If this channel is closed
      *
+     * @throws  ClosedSelectorException
+     *          If the selector is closed
+     *
      * @throws  IllegalBlockingModeException
      *          If this channel is in blocking mode
      *

src/share/classes/java/nio/channels/spi/AbstractSelectableChannel.java

@@ -175,6 +175,16 @@ public abstract class AbstractSelectableChannel
      * the selector is invoked while holding the appropriate locks.  The
      * resulting key is added to this channel's key set before being returned.
      * </p>
+     *
+     * @throws  ClosedSelectorException {@inheritDoc}
+     *
+     * @throws  IllegalBlockingModeException {@inheritDoc}
+     *
+     * @throws  IllegalSelectorException {@inheritDoc}
+     *
+     * @throws  CancelledKeyException {@inheritDoc}
+     *
+     * @throws  IllegalArgumentException {@inheritDoc}
      */
     public final SelectionKey register(Selector sel, int ops,
                                        Object att)

src/share/classes/sun/nio/ch/AbstractPollSelectorImpl.java

@@ -58,6 +58,9 @@ abstract class AbstractPollSelectorImpl
     // True if this Selector has been closed
     private boolean closed = false;
 
+    // Lock for close and cleanup
+    private Object closeLock = new Object();
+
     AbstractPollSelectorImpl(SelectorProvider sp, int channels, int offset) {
         super(sp);
         this.totalChannels = channels;
@@ -65,7 +68,11 @@ abstract class AbstractPollSelectorImpl
     }
 
     void putEventOps(SelectionKeyImpl sk, int ops) {
-        pollWrapper.putEventOps(sk.getIndex(), ops);
+        synchronized (closeLock) {
+            if (closed)
+                throw new ClosedSelectorException();
+            pollWrapper.putEventOps(sk.getIndex(), ops);
+        }
     }
 
     public Selector wakeup() {
@@ -76,7 +83,9 @@ abstract class AbstractPollSelectorImpl
     protected abstract int doSelect(long timeout) throws IOException;
 
     protected void implClose() throws IOException {
-        if (!closed) {
+        synchronized (closeLock) {
+            if (closed)
+                return;
             closed = true;
             // Deregister channels
             for(int i=channelOffset; i<totalChannels; i++) {
@@ -129,23 +138,28 @@ abstract class AbstractPollSelectorImpl
     }
 
     protected void implRegister(SelectionKeyImpl ski) {
-        // Check to see if the array is large enough
-        if (channelArray.length == totalChannels) {
-            // Make a larger array
-            int newSize = pollWrapper.totalChannels * 2;
-            SelectionKeyImpl temp[] = new SelectionKeyImpl[newSize];
-            // Copy over
-            for (int i=channelOffset; i<totalChannels; i++)
-                temp[i] = channelArray[i];
-            channelArray = temp;
-            // Grow the NativeObject poll array
-            pollWrapper.grow(newSize);
+        synchronized (closeLock) {
+            if (closed)
+                throw new ClosedSelectorException();
+
+            // Check to see if the array is large enough
+            if (channelArray.length == totalChannels) {
+                // Make a larger array
+                int newSize = pollWrapper.totalChannels * 2;
+                SelectionKeyImpl temp[] = new SelectionKeyImpl[newSize];
+                // Copy over
+                for (int i=channelOffset; i<totalChannels; i++)
+                    temp[i] = channelArray[i];
+                channelArray = temp;
+                // Grow the NativeObject poll array
+                pollWrapper.grow(newSize);
+            }
+            channelArray[totalChannels] = ski;
+            ski.setIndex(totalChannels);
+            pollWrapper.addEntry(ski.channel);
+            totalChannels++;
+            keys.add(ski);
         }
-        channelArray[totalChannels] = ski;
-        ski.setIndex(totalChannels);
-        pollWrapper.addEntry(ski.channel);
-        totalChannels++;
-        keys.add(ski);
     }
 
     protected void implDereg(SelectionKeyImpl ski) throws IOException {

src/solaris/classes/sun/nio/ch/DevPollSelectorImpl.java

@@ -46,15 +46,15 @@ class DevPollSelectorImpl
     // The poll object
     DevPollArrayWrapper pollWrapper;
 
-    // The number of valid channels in this Selector's poll array
-    private int totalChannels;
-
     // Maps from file descriptors to keys
     private Map<Integer,SelectionKeyImpl> fdToKey;
 
     // True if this Selector has been closed
     private boolean closed = false;
 
+    // Lock for close/cleanup
+    private Object closeLock = new Object();
+
     // Lock for interrupt triggering and clearing
     private Object interruptLock = new Object();
     private boolean interruptTriggered = false;
@@ -72,7 +72,6 @@ class DevPollSelectorImpl
         pollWrapper = new DevPollArrayWrapper();
         pollWrapper.initInterrupt(fd0, fd1);
         fdToKey = new HashMap<Integer,SelectionKeyImpl>();
-        totalChannels = 1;
     }
 
     protected int doSelect(long timeout)
@@ -131,45 +130,39 @@ class DevPollSelectorImpl
     }
 
     protected void implClose() throws IOException {
-        if (!closed) {
-            closed = true;
-
-            // prevent further wakeup
-            synchronized (interruptLock) {
-                interruptTriggered = true;
-            }
+        if (closed)
+            return;
+        closed = true;
 
-            FileDispatcher.closeIntFD(fd0);
-            FileDispatcher.closeIntFD(fd1);
-            if (pollWrapper != null) {
-
-                pollWrapper.release(fd0);
-                pollWrapper.closeDevPollFD();
-                pollWrapper = null;
-                selectedKeys = null;
-
-                // Deregister channels
-                Iterator i = keys.iterator();
-                while (i.hasNext()) {
-                    SelectionKeyImpl ski = (SelectionKeyImpl)i.next();
-                    deregister(ski);
-                    SelectableChannel selch = ski.channel();
-                    if (!selch.isOpen() && !selch.isRegistered())
-                        ((SelChImpl)selch).kill();
-                    i.remove();
-                }
-                totalChannels = 0;
+        // prevent further wakeup
+        synchronized (interruptLock) {
+            interruptTriggered = true;
+        }
 
-            }
-            fd0 = -1;
-            fd1 = -1;
+        FileDispatcher.closeIntFD(fd0);
+        FileDispatcher.closeIntFD(fd1);
+
+        pollWrapper.release(fd0);
+        pollWrapper.closeDevPollFD();
+        selectedKeys = null;
+
+        // Deregister channels
+        Iterator i = keys.iterator();
+        while (i.hasNext()) {
+            SelectionKeyImpl ski = (SelectionKeyImpl)i.next();
+            deregister(ski);
+            SelectableChannel selch = ski.channel();
+            if (!selch.isOpen() && !selch.isRegistered())
+                ((SelChImpl)selch).kill();
+            i.remove();
         }
+        fd0 = -1;
+        fd1 = -1;
     }
 
     protected void implRegister(SelectionKeyImpl ski) {
         int fd = IOUtil.fdVal(ski.channel.getFD());
         fdToKey.put(Integer.valueOf(fd), ski);
-        totalChannels++;
         keys.add(ski);
     }
 
@@ -179,7 +172,6 @@ class DevPollSelectorImpl
         int fd = ski.channel.getFDVal();
         fdToKey.remove(Integer.valueOf(fd));
         pollWrapper.release(fd);
-        totalChannels--;
         ski.setIndex(-1);
         keys.remove(ski);
         selectedKeys.remove(ski);
@@ -190,6 +182,8 @@ class DevPollSelectorImpl
     }
 
     void putEventOps(SelectionKeyImpl sk, int ops) {
+        if (closed)
+            throw new ClosedSelectorException();
         int fd = IOUtil.fdVal(sk.channel.getFD());
         pollWrapper.setInterest(fd, ops);
     }

src/solaris/classes/sun/nio/ch/EPollSelectorImpl.java

 /*
- * Copyright 2005-2008 Sun Microsystems, Inc.  All Rights Reserved.
+ * Copyright 2005-2007 Sun Microsystems, Inc.  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
@@ -31,7 +31,6 @@ import java.nio.channels.spi.*;
 import java.util.*;
 import sun.misc.*;
 
-
 /**
  * An implementation of Selector for Linux 2.6+ kernels that uses
  * the epoll event notification facility.
@@ -51,7 +50,7 @@ class EPollSelectorImpl
     private Map<Integer,SelectionKeyImpl> fdToKey;
 
     // True if this Selector has been closed
-    private boolean closed = false;
+    private volatile boolean closed = false;
 
     // Lock for interrupt triggering and clearing
     private Object interruptLock = new Object();
@@ -128,40 +127,41 @@ class EPollSelectorImpl
     }
 
     protected void implClose() throws IOException {
-        if (!closed) {
-            closed = true;
+        if (closed)
+            return;
+        closed = true;
 
-            // prevent further wakeup
-            synchronized (interruptLock) {
-                interruptTriggered = true;
-            }
+        // prevent further wakeup
+        synchronized (interruptLock) {
+            interruptTriggered = true;
+        }
 
-            FileDispatcher.closeIntFD(fd0);
-            FileDispatcher.closeIntFD(fd1);
-            if (pollWrapper != null) {
-
-                pollWrapper.release(fd0);
-                pollWrapper.closeEPollFD();
-                pollWrapper = null;
-                selectedKeys = null;
-
-                // Deregister channels
-                Iterator i = keys.iterator();
-                while (i.hasNext()) {
-                    SelectionKeyImpl ski = (SelectionKeyImpl)i.next();
-                    deregister(ski);
-                    SelectableChannel selch = ski.channel();
-                    if (!selch.isOpen() && !selch.isRegistered())
-                        ((SelChImpl)selch).kill();
-                    i.remove();
-                }
-            }
-            fd0 = -1;
-            fd1 = -1;
+        FileDispatcher.closeIntFD(fd0);
+        FileDispatcher.closeIntFD(fd1);
+
+        pollWrapper.release(fd0);
+        pollWrapper.closeEPollFD();
+        // it is possible
+        selectedKeys = null;
+
+        // Deregister channels
+        Iterator i = keys.iterator();
+        while (i.hasNext()) {
+            SelectionKeyImpl ski = (SelectionKeyImpl)i.next();
+            deregister(ski);
+            SelectableChannel selch = ski.channel();
+            if (!selch.isOpen() && !selch.isRegistered())
+                ((SelChImpl)selch).kill();
+            i.remove();
         }
+
+        fd0 = -1;
+        fd1 = -1;
     }
 
     protected void implRegister(SelectionKeyImpl ski) {
+        if (closed)
+            throw new ClosedSelectorException();
         int fd = IOUtil.fdVal(ski.channel.getFD());
         fdToKey.put(Integer.valueOf(fd), ski);
         pollWrapper.add(fd);
@@ -183,6 +183,8 @@ class EPollSelectorImpl
     }
 
     void putEventOps(SelectionKeyImpl sk, int ops) {
+        if (closed)
+            throw new ClosedSelectorException();
         int fd = IOUtil.fdVal(sk.channel.getFD());
         pollWrapper.setInterest(fd, ops);
     }

src/windows/classes/sun/nio/ch/WindowsSelectorImpl.java

 /*
- * Copyright 2002-2008 Sun Microsystems, Inc.  All Rights Reserved.
+ * Copyright 2002-2007 Sun Microsystems, Inc.  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
@@ -80,6 +80,9 @@ final class WindowsSelectorImpl extends SelectorImpl {
     // File descriptors corresponding to source and sink
     private final int wakeupSourceFd, wakeupSinkFd;
 
+    // Lock for close cleanup
+    private Object closeLock = new Object();
+
     // Maps file descriptors to their indices in  pollArray
     private final static class FdMap extends HashMap<Integer, MapEntry> {
         static final long serialVersionUID = 0L;
@@ -473,42 +476,48 @@ final class WindowsSelectorImpl extends SelectorImpl {
     }
 
     protected void implClose() throws IOException {
-        if (channelArray != null) {
-            if (pollWrapper != null) {
-                // prevent further wakeup
-                synchronized (interruptLock) {
-                    interruptTriggered = true;
-                }
-                wakeupPipe.sink().close();
-                wakeupPipe.source().close();
-                for(int i = 1; i < totalChannels; i++) { // Deregister channels
-                    if (i % MAX_SELECTABLE_FDS != 0) { // skip wakeupEvent
-                        deregister(channelArray[i]);
-                        SelectableChannel selch = channelArray[i].channel();
-                        if (!selch.isOpen() && !selch.isRegistered())
-                            ((SelChImpl)selch).kill();
+        synchronized (closeLock) {
+            if (channelArray != null) {
+                if (pollWrapper != null) {
+                    // prevent further wakeup
+                    synchronized (interruptLock) {
+                        interruptTriggered = true;
                     }
-                }
-                pollWrapper.free();
-                pollWrapper = null;
-                selectedKeys = null;
-                channelArray = null;
-                threads.clear();
-                // Call startThreads. All remaining helper threads now exit,
-                // since threads.size() = 0;
-                startLock.startThreads();
+                    wakeupPipe.sink().close();
+                    wakeupPipe.source().close();
+                    for(int i = 1; i < totalChannels; i++) { // Deregister channels
+                        if (i % MAX_SELECTABLE_FDS != 0) { // skip wakeupEvent
+                            deregister(channelArray[i]);
+                            SelectableChannel selch = channelArray[i].channel();
+                            if (!selch.isOpen() && !selch.isRegistered())
+                                ((SelChImpl)selch).kill();
+                        }
+                    }
+                    pollWrapper.free();
+                    pollWrapper = null;
+                     selectedKeys = null;
+                     channelArray = null;
+                     threads.clear();
+                     // Call startThreads. All remaining helper threads now exit,
+                     // since threads.size() = 0;
+                     startLock.startThreads();
+                 }
             }
         }
     }
 
     protected void implRegister(SelectionKeyImpl ski) {
-        growIfNeeded();
-        channelArray[totalChannels] = ski;
-        ski.setIndex(totalChannels);
-        fdMap.put(ski);
-        keys.add(ski);
-        pollWrapper.addEntry(totalChannels, ski);
-        totalChannels++;
+        synchronized (closeLock) {
+            if (pollWrapper == null)
+                throw new ClosedSelectorException();
+            growIfNeeded();
+            channelArray[totalChannels] = ski;
+            ski.setIndex(totalChannels);
+            fdMap.put(ski);
+            keys.add(ski);
+            pollWrapper.addEntry(totalChannels, ski);
+            totalChannels++;
+        }
     }
 
     private void growIfNeeded() {
@@ -554,7 +563,11 @@ final class WindowsSelectorImpl extends SelectorImpl {
     }
 
     void putEventOps(SelectionKeyImpl sk, int ops) {
-        pollWrapper.putEventOps(sk.getIndex(), ops);
+        synchronized (closeLock) {
+            if (pollWrapper == null)
+                throw new ClosedSelectorException();
+            pollWrapper.putEventOps(sk.getIndex(), ops);
+        }
     }
 
     public Selector wakeup() {

test/java/nio/channels/Selector/CloseThenRegister.java

+/*
+ * Copyright 2008 Sun Microsystems, Inc.  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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
+ * CA 95054 USA or visit www.sun.com if you need additional information or
+ * have any questions.
+ */
+
+/* @test
+ * @bug 5025260
+ * @summary ClosedSelectorException is expected when register after close
+ */
+
+import java.net.*;
+import java.nio.channels.*;
+
+public class CloseThenRegister {
+
+    public static void main (String [] args) throws Exception {
+        try {
+            Selector s = Selector.open();
+            s.close();
+            ServerSocketChannel c = ServerSocketChannel.open();
+            c.socket().bind(new InetSocketAddress(40000));
+            c.configureBlocking(false);
+            c.register(s, SelectionKey.OP_ACCEPT);
+        } catch (ClosedSelectorException cse) {
+            return;
+        }
+        throw new RuntimeException("register after close does not cause CSE!");
+    }
+
+}