How correctly close SocketChannel in Java NIO?

Question

I have a simple non-blocking server with main loop:

try {
    while (selector.select() > -1) {

        // Wait for an event one of the registered channels

        // Iterate over the set of keys for which events are available
        Iterator selectedKeys = selector.selectedKeys().iterator();
        while (selectedKeys.hasNext()) {
            SelectionKey key = (SelectionKey) selectedKeys.next();
            selectedKeys.remove();
            try {
                if (!key.isValid()) {
                    continue;
                }

                if (key.isConnectable()) {
                    connect(key);
                }

                // Check what event is available and deal with it
                if (key.isAcceptable()) {
                    accept(key);
                }

                if (key.isReadable()) {
                    read(key);
                }

                if (key.isWritable()) {
                    write(key);
                }
            } catch (Exception e) {
                e.printStackTrace();
                close(key);
            }
        }
    }
} catch (Exception e) {
    e.printStackTrace();
}

In read/write section I check if there is something to read/write if not - then I try to close channel:

if (channel.read(attachment.buffer) < 1) 
    close(key);

Close method:

private void close(SelectionKey key) throws IOException {
    key.cancel();
    key.channel().close();
}

But during processing this code I get exception in main loop (it is catched but I supposed something wrong) I get this stacktrace:

java.nio.channels.CancelledKeyException
    at sun.nio.ch.SelectionKeyImpl.ensureValid(Unknown Source)
    at sun.nio.ch.SelectionKeyImpl.readyOps(Unknown Source)
    at java.nio.channels.SelectionKey.isWritable(Unknown Source)

So it fails on main loop when enter write section, close channel and came back to main loop in 'writable' if section and fails with such exception. Any suggestions?

Solution

The error is very simple.

if (!key.isValid()) {
    continue;
}

if (key.isConnectable()) {
    connect(key);
}

// Check what event is available and deal with it
if (key.isAcceptable()) {
    accept(key);
}

if (key.isReadable()) {
    read(key);
}

if (key.isWritable()) {
    write(key);
}

Your read method is the one which cancels the SelectionKey. However, after returning from read, you again test the key for whether the channel is writable -- potentially after just cancelling that very same key! Your initial check cannot help here.

---

One solution would be to check for whether the key is valid wherever it might've just been cancelled:

...
if (key.isValid() && key.isWritable()) {
  write(key);
}
...

---

Alternatively, you could also try only registering one interest at a time as you need to on any particular channel, and thus all readiness events are mutually exclusive:

if (!key.isValid()) {
  continue;
}

if (key.isConnectable()) {
  connect(key);
} else if (key.isAcceptable()) {
  accept(key);
} else if (key.isReadable()) {
  read(key);
} else if (key.isWritable()) {
  write(key);
}

This might be beneficial in situations; as generally a channel will almost always be write-ready, keeping an interest in write-readiness along side read-readiness might keep the Selector loop spinning, which is more than likely not desirable. For the most part, generally register interest in write-readiness only when the underlying socket output buffer is full.

---

As a side note, know that SocketChannel.read can return a value < 1 without it being an error.

A read operation might not fill the buffer, and in fact it might not read any bytes at all. Whether or not it does so depends upon the nature and state of the channel. A socket channel in non-blocking mode, for example, cannot read any more bytes than are immediately available from the socket's input buffer;

Additionally, Selector.select does not state anything about returning < -1 to indicate it is closed.

Returns: The number of keys, possibly zero, whose ready-operation sets were updated

Comments

[sphinks]

Thanks. So how will be correct to stop server loop? On what condition?

[sphinks]

And make main loop in this way: if (key.isConnectable()) { connect(key); } else if (key.isAcceptable()) { accept(key); } else if (key.isReadable()) { read(key); } else if (key.isWritable()) { write(key); }

[obataku]

@sphinks you should use Selector.close; this will internally interrupt the event loop thread if it is blocking on select. You can test whether a Selector is open by using Selector.isOpen. If you attempt to select using a closed Selector, it will throw a ClosedSelectorException.

[user207421]

@sphinks It might make sense to loop on while (selector.keys().size() > 0), which will exit when all the channels under management have been closed. Then close the selector after the loop.