charlie hunt wrote:
> 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.
Interesting :-) Can you file an issue so we don't forget it?
>
> 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.
Wow :-) You need to work on Grizzly more often :-)
Comment inline...
>
> 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;
Stupid question. Not initializing the variable makes a difference?
> 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);
> /*
> * The contents of this file are subject to the terms
> * of the Common Development and Distribution License
> * (the License). You may not use this file except in
> * compliance with the License.
> *
> * You can obtain a copy of the license at
> * https://glassfish.dev.java.net/public/CDDLv1.0.html or
> * glassfish/bootstrap/legal/CDDLv1.0.txt.
> * See the License for the specific language governing
> * permissions and limitations under the License.
> *
> * When distributing Covered Code, include this CDDL
> * Header Notice in each file and include the License file
> * at glassfish/bootstrap/legal/CDDLv1.0.txt.
> * If applicable, add the following below the CDDL Header,
> * with the fields enclosed by brackets [] replaced by
> * you own identifying information:
> * "Portions Copyrighted [year] [name of copyright owner]"
> *
> * Copyright 2006-2008 Sun Microsystems, Inc. All rights reserved.
> */
>
> package com.sun.grizzly;
>
> import com.sun.grizzly.util.Copyable;
> import com.sun.grizzly.util.SelectionKeyActionAttachment;
> 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.Selector;
> import java.nio.channels.SocketChannel;
> import java.util.Iterator;
> import java.util.logging.Logger;
>
> /**
> *
Some description :-)
> * @author Charlie Hunt
> */
> public class BaseSelectionKeyHandler implements SelectionKeyHandler {
>
> protected Logger logger = Controller.logger();
>
>
> /**
> * Associated <code>SelectorHandler</code>
> */
> protected SelectorHandler selectorHandler;
>
>
> public BaseSelectionKeyHandler() {
> }
>
>
> public BaseSelectionKeyHandler(SelectorHandler selectorHandler) {
> this.selectorHandler = selectorHandler;
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public SelectorHandler getSelectorHandler() {
> return selectorHandler;
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public void setSelectorHandler(SelectorHandler selectorHandler) {
> this.selectorHandler = selectorHandler;
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public void process(SelectionKey key) {
> Object attachment = key.attachment();
>
> // NOTE: If attachment == null, instanceof will evaluate to false.
> // If attachment == null does not generate NullPointerException
> if (attachment instanceof SelectionKeyActionAttachment) {
> ((SelectionKeyActionAttachment) attachment).process(key);
> }
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public void postProcess(SelectionKey key) {
> // Currently nothing to do here.
> }
>
>
> /**
> * @deprecated
> */
> public void register(SelectionKey key, long currentTime) {
> throw new UnsupportedOperationException(getClass().getName() +
> " use of this API not allowed.");
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public void register(SelectionKey key, int selectionKeyOps) {
> doRegisterKey(key, selectionKeyOps);
> }
>
>
> /**
> * Registers <code>SelectionKey</code> to handle certain operations
> */
> protected void doRegisterKey(SelectionKey key, int selectionKeyOps) {
> if (!key.isValid()) {
> return;
> }
> key.interestOps(key.interestOps() | selectionKeyOps);
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public void register(SelectableChannel channel, int selectionKeyOps) throws ClosedChannelException {
> if (!channel.isOpen()) {
> return;
> }
> Selector selector = selectorHandler.getSelector();
> SelectionKey key = channel.keyFor(selector);
> if (key == null) {
> channel.register(selector, selectionKeyOps);
> } else {
> doRegisterKey(key, selectionKeyOps);
> }
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public void register(Iterator<SelectionKey> keyIterator, int selectionKeyOps) {
> SelectionKey key;
> while (keyIterator.hasNext()) {
> key = keyIterator.next();
> keyIterator.remove();
> doRegisterKey(key, selectionKeyOps);
> }
> }
>
>
> /**
> * @deprecated
> */
> public void expire(SelectionKey key, long currentTime) {
> // Do nothing.
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public void expire(Iterator<SelectionKey> keyIterator) {
> // Do nothing.
> }
>
>
> /**
> * Cancel a SelectionKey and close its associated Channel.
> * @param key <code>SelectionKey</code> to cancel
> */
> public void cancel(SelectionKey key) {
> if (!keyIsValid(key)) {
> return;
> }
>
> closeChannel(key);
>
> clearKeyAttachment(key);
>
> cancelKey(key);
> }
>
>
> /**
> * {_at_inheritDoc}
> */
> public void close(SelectionKey key) {
> cancel(key);
> }
>
> /**
> * {_at_inheritDoc}
> */
> public void copyTo(Copyable copy) {
> BaseSelectionKeyHandler copyHandler = (BaseSelectionKeyHandler) copy;
> copyHandler.selectorHandler = selectorHandler;
> }
>
>
> public Logger getLogger() {
> return logger;
> }
>
>
> public void setLogger(Logger logger) {
> this.logger = logger;
> }
>
>
> protected void cancelKey(SelectionKey key) {
> key.cancel();
> key = null;
> }
>
>
> protected boolean keyIsValid(SelectionKey key) {
> if (key != null && key.isValid()) {
> return true;
> } else {
> return false;
> }
> }
>
>
> protected void closeChannel(SelectionKey key) {
> if (selectorHandler != null) {
> selectorHandler.closeChannel(key.channel());
> } else {
> closeChannel(key.channel());
> }
> }
>
>
> protected Object clearKeyAttachment(SelectionKey key) {
> return key.attach(null);
> }
>
>
> @SuppressWarnings("empty-statement")
> 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){
> ;
> }
> }
>
> try{
> channel.close();
> } catch (IOException ex){
> ; // LOG ME
> }
Since we are refactoring, what do you think about logging all
IOException here to FINEST?
Thanks!
-- Jeanfrancois
> }
> }