I have finished refactoring DefaultSelectionKeyHandler into a
BaseSelectionKeyHandler. BaseSelectionKeyHandler can now be used as a
SelectionKeyHandler when a person does not want, or need to expire
connections, (i.e. apply keep alive logic).
I also have a unit test case for it too. :-)
Since this is a change that touches some pretty important files, I would
like to get a review of the changes before I commit them to the trunk.
A couple quick notes:
1.) I am attaching the new BaseSelectionKeyHandler.java file and unit
test case for the changes along with the svn diffs for changes to
DefaultSelectionKeyHandler. I also made a couple changes to
TCPSelectorHandler.java which will remove a couple load instructions
which may save a some CPU cycles on a couple "hot' code paths.
2.) I am not 100% happy with the refactoring since the
SelectionKeyHandler interface implies expiring keys with its expire()
method. And, this notion also propagates into
TCPSelectorHandler.postSelect(). To be 100% happy, a
SelectionKeyHandler interface and TCPSelectorHandler.postSelect() would
need to be refactored. I don't have a problem with refactoring
TCPSelectorHandler, but I don't like the idea of refactoring
SelectionKeyHandler. I would summarize the issue here as; the notion
of expiring keys should be a more general "post select" activity and not
forced upon a user of a Grizzly transport when he / she does not need
expiring keys support. We might consider addressing this in Grizzly 2.0
also.
3.) I may want to do some looking at the JIT compiled generated code to
see if when using the BaseSelectionkeyHandler, check if the
TCPSelectorHandler.postSelect() gets optimized into a no-op since
there's nothing to do on a postSelect() when using the
BaseSelectionKeyHandler. If the JIT compiler can inline all of what is
called in TCPSelectorHandler.postSelect(), the JIT compiler may be able
to optimize it into a no-op. But, it may not be able to inline it and
optimize it into a no-op if there's more than one implementation of a
SelectionKeyHandler observed by the JIT compiler at run time. I think
this may help identify the urgency to address further refactoring
SelectionKeyHandler and TCPSelectorHandler.
New source code, test case and diffs attached.
charlie ...
Index: main/java/com/sun/grizzly/DefaultSelectionKeyHandler.java
===================================================================
--- main/java/com/sun/grizzly/DefaultSelectionKeyHandler.java (revision 840)
+++ main/java/com/sun/grizzly/DefaultSelectionKeyHandler.java (working copy)
@@ -18,25 +18,20 @@
* you own identifying information:
* "Portions Copyrighted [year] [name of copyright owner]"
*
- * Copyright 2006 Sun Microsystems, Inc. All rights reserved.
+ * Copyright 2006-2008 Sun Microsystems, Inc. All rights reserved.
*/
package com.sun.grizzly;
import com.sun.grizzly.util.SelectionKeyAttachment;
-import com.sun.grizzly.util.SelectionKeyActionAttachment;
import com.sun.grizzly.util.Copyable;
-import java.io.IOException;
-import java.net.Socket;
import java.nio.channels.ClosedChannelException;
import java.nio.channels.SelectableChannel;
import java.nio.channels.SelectionKey;
-import java.nio.channels.SocketChannel;
+import java.nio.channels.Selector;
import java.util.Iterator;
import java.util.logging.Level;
-import java.util.logging.Logger;
-import java.nio.channels.Selector;
/**
* Default implementation of a SelectionKey Handler. By default, this
@@ -48,12 +43,9 @@
*
* @author Jeanfrancois Arcand
*/
-public class DefaultSelectionKeyHandler implements SelectionKeyHandler {
-
-
- protected Logger logger = Controller.logger();
-
-
+public class DefaultSelectionKeyHandler extends BaseSelectionKeyHandler {
+
+
/**
* Next time the exprireKeys() will delete keys.
*/
@@ -64,55 +56,44 @@
* Number of seconds before idle keep-alive connections expire
*/
protected long timeout = 30 * 1000L;
+
- /**
- * Associated <code>SelectorHandler</code>
- */
- private SelectorHandler selectorHandler;
-
public DefaultSelectionKeyHandler() {
}
+
public DefaultSelectionKeyHandler(SelectorHandler selectorHandler) {
- this.selectorHandler = selectorHandler;
+ super(selectorHandler);
}
+
+ /**
+ * {_at_inheritDoc}
+ */
+ @Override
public void copyTo(Copyable copy) {
DefaultSelectionKeyHandler copyHandler = (DefaultSelectionKeyHandler) copy;
copyHandler.selectorHandler = selectorHandler;
}
- /**
- * {_at_inheritDoc}
- */
- public SelectorHandler getSelectorHandler() {
- return selectorHandler;
- }
- /**
- * {_at_inheritDoc}
- */
- public void setSelectorHandler(SelectorHandler selectorHandler) {
- this.selectorHandler = selectorHandler;
- }
+
/**
* {_at_inheritDoc}
*/
+ @Override
public void process(SelectionKey key) {
- Object attachment = key.attachment();
-
- if (attachment instanceof SelectionKeyActionAttachment) {
- ((SelectionKeyActionAttachment) attachment).process(key);
- }
-
+ super.process(key);
removeExpirationStamp(key);
}
/**
* {_at_inheritDoc}
*/
+ @Override
public void postProcess(SelectionKey key) {
+ super.postProcess(key);
addExpirationStamp(key);
}
@@ -120,13 +101,14 @@
/**
* {_at_inheritDoc}
*/
- public void register(Iterator<SelectionKey> iterator, int ops) {
+ @Override
+ public void register(Iterator<SelectionKey> keyIterator, int selectionKeyOps) {
long currentTime = System.currentTimeMillis();
SelectionKey key;
- while (iterator.hasNext()) {
- key = iterator.next();
- iterator.remove();
- doRegisterKey(key, ops, currentTime);
+ while (keyIterator.hasNext()) {
+ key = keyIterator.next();
+ keyIterator.remove();
+ doRegisterKey(key, selectionKeyOps, currentTime);
}
}
@@ -134,6 +116,7 @@
/**
* {_at_inheritDoc}
*/
+ @Override
public void register(SelectionKey key, int selectionKeyOps) {
doRegisterKey(key, selectionKeyOps, System.currentTimeMillis());
}
@@ -158,6 +141,7 @@
/**
* {_at_inheritDoc}
*/
+ @Override
public void register(SelectableChannel channel, int ops)
throws ClosedChannelException {
if (!channel.isOpen()) {
@@ -173,12 +157,13 @@
} else {
doRegisterKey(key, ops, time);
}
-
}
/**
* {_at_inheritDoc}
*/
+ @Override
+ @SuppressWarnings("empty-statement")
public void register(SelectionKey key, long currentTime){
;
}
@@ -187,6 +172,8 @@
/**
* @deprecated
*/
+ @Override
+ @SuppressWarnings("empty-statement")
public void expire(SelectionKey key, long currentTime) {
;
}
@@ -195,6 +182,7 @@
/**
* {_at_inheritDoc}
*/
+ @Override
public void expire(Iterator<SelectionKey> iterator) {
if (timeout <= 0) return;
@@ -226,46 +214,6 @@
}
- /**
- * Cancel a SelectionKey and close its associated Channel.
- * @param key <code>SelectionKey</code> to cancel
- */
- public void cancel(SelectionKey key) {
- if (key == null || !key.isValid()) {
- return;
- }
-
- if (selectorHandler != null) {
- selectorHandler.closeChannel(key.channel());
- } else {
- closeChannel(key.channel());
- }
-
- Object attachment = key.attach(null);
- if (attachment instanceof SelectionKeyAttachment) {
- ((SelectionKeyAttachment) attachment).release(key);
- }
-
- key.cancel();
- key = null;
- }
-
-
- public void close(SelectionKey key) {
- cancel(key);
- }
-
-
- public Logger getLogger() {
- return logger;
- }
-
-
- public void setLogger(Logger logger) {
- this.logger = logger;
- }
-
-
public long getTimeout() {
return timeout;
}
@@ -275,36 +223,17 @@
this.timeout = timeout;
}
- protected void closeChannel(SelectableChannel channel) {
- if (channel instanceof SocketChannel) {
- Socket socket = ((SocketChannel) channel).socket();
-
- try{
- if (!socket.isInputShutdown()) socket.shutdownInput();
- } catch (IOException ex){
- ;
- }
-
- try{
- if (!socket.isOutputShutdown()) socket.shutdownOutput();
- } catch (IOException ex){
- ;
- }
-
- try{
- socket.close();
- } catch (IOException ex){
- ;
- }
+
+ @Override
+ protected Object clearKeyAttachment(SelectionKey key) {
+ Object attachment = super.clearKeyAttachment(key);
+ if (attachment instanceof SelectionKeyAttachment) {
+ ((SelectionKeyAttachment) attachment).release(key);
}
-
- try{
- channel.close();
- } catch (IOException ex){
- ; // LOG ME
- }
+ return attachment;
}
-
+
+
/**
* Removes expiration timeout stamp from the <code>SelectionKey</code>
* depending on its attachment
Index: main/java/com/sun/grizzly/TCPSelectorHandler.java
===================================================================
--- main/java/com/sun/grizzly/TCPSelectorHandler.java (revision 840)
+++ main/java/com/sun/grizzly/TCPSelectorHandler.java (working copy)
@@ -407,7 +407,7 @@
SelectionKey.OP_CONNECT);
key.attach(CallbackHandlerSelectionKeyAttachment.create(key, callbackHandler));
- boolean isConnected = false;
+ boolean isConnected;
try {
isConnected = socketChannel.connect(remoteAddress);
} catch(IOException e) {
@@ -653,7 +653,7 @@
// disable OP_WRITE on key before doing anything else
key.interestOps(key.interestOps() & (~SelectionKey.OP_WRITE));
- Object attach = null;
+ Object attach;
if (asyncQueueWriter.hasReadyAsyncWriteData(key)) {
final Context context = pollContext(ctx, key);