Hi Alexey,
Thanks for quick patch!
I have already tested your fix and it works well. :-)
In addition,
Bongjae wrote:
I think that ConnectorHandler's socketChannel can be null if unexpected exceptions are occurred in CallbackHandler. ex) if an error was occurred before calling ConnectorHandler#finishConnect(SelectionKey key)
So I think that graceful handling of NPE and more check logic are needed about underlyingConnectorHandler.getUnderlyingChannel().
In CacheableConnectorHandler.java
-----
private voide notifyCallbackHandlerPseudoConnect() throws ClosedChannelException {
...
SelectionKey key = underlyingConnectorHandler.getUnderlyingChannel().keyFor( protocolSelector );
...
}
At above the bold line, don't we need to check null channel? Is it redundant?
If user makes a wrong CallbackHandler impl and AbstractConnectorHandler#setUnderlyingChannel doesn't be called, "underlyingConnectorHandler#getUnderlyingChannel()" can return null's channel adequately.
So I think that graceful exception or at least warning log should be printed rather than NPE like this.
-----
private voide notifyCallbackHandlerPseudoConnect() throws IOException {
SelectableChannel underlyingChannel = underlyingConnectorHandler.getUnderlyingChannel();
if( underlyingChannel == null ) {
throw new IOException(); // warning log or exception
}
SelectionKey key = underlyingChannel.keyFor( protocolSelector );
...
}
-----
Thanks.
--
Bongjae Chang
----- Original Message -----
From: Oleksiy Stashok
To: dev_at_grizzly.dev.java.net
Sent: Tuesday, April 28, 2009 10:15 PM
Subject: Re: Connection management cache's problems
Hi Bongjae,
Thank you for finding this out!
I've commited the fix, please let me know if it works for you.
WBR,
Alexey.
On Apr 28, 2009, at 6:38 , Bongjae Chang wrote:
In addition, default CallbackHandler in CacheableConnectorHandler didn't implement onRead(), onWrite().
Here is default CallbackHandler in CacheableConnectorHandler.java
-----
public void onConnect(IOEvent ioEvent) {
if (underlyingCallbackHandler == null) {
underlyingCallbackHandler = new CallbackHandler<Context>(){
public void onConnect(IOEvent<Context> ioEvent) {
SelectionKey key = ioEvent.attachment().getSelectionKey();
finishConnect(key);
ioEvent.attachment().getSelectorHandler().register(key,SelectionKey.OP_READ);
}
public void onRead(IOEvent<Context> ioEvent) {}
public void onWrite(IOEvent<Context> ioEvent) {}
};
}
underlyingCallbackHandler.onConnect(ioEvent);
}
-----
So I couldn't test EchoFilter with CacheableConnectorHandlerPool. Are there any restrictions?
If not, I think that the following code is better(Of course, if DefaultCallbackHandler doesn't have ClassCastException's error).
-----
public void onConnect(IOEvent ioEvent) {
if (underlyingCallbackHandler == null) {
underlyingCallbackHandler = new DefaultCallbackHandler( this );
}
underlyingCallbackHandler.onConnect(ioEvent);
}
-----
Thanks.
--
Bongjae Chang
----- Original Message -----
From: Bongjae Chang
To: dev_at_grizzly.dev.java.net
Sent: Tuesday, April 28, 2009 11:31 AM
Subject: Connection management cache's problems
Hi.
I tried to test com.sun.grizzly.connectioncache.client.CacheableConnectorHandlerPool with com.sun.grizzly.DefaultCallbackHandler, but I had some problems.
Here is simple test code.
-----
[init part]
Controller controller = new Controller();
ConnectorHandlerPool cacheableHandlerPool = new CacheableConnectorHandlerPool( controller, 100, 10, 1 );
controller.setConnectorHandlerPool( cacheableHandlerPool );
TCPSelectorHandler tcpSelectorHandler = new TCPSelectorHandler();
tcpSelectorHandler.setPort( myPort );
tcpSelectorHandler.setSelectionKeyHandler( new DefaultSelectionKeyHandler() );
controller.addSelectorHandler( tcpSelectorHandler );
...
[send part]
ConnectorHandler connectorHandler;
connectorHandler = controller.acquireConnectorHandler( Controller.Protocol.TCP );
..
connectorHandler.connect( remoteAddress, localAddress, new DefaultCallbackHandler( connectorHandler ) );
OutputWrite.flushChannel( connectorHandler.getUnderlyingChannel(), appMessage, timeout );
...
-----
Here is result.
-----
java.lang.IllegalStateException: Invalid Response State. SocketChannel cannot be null.
at com.sun.grizzly.util.OutputWriter.flushChannel(OutputWriter.java:94)
at SimpleTestServer.sendTest(SimpleTestServer.java:134)
at SimpleTestServer.main(SimpleTestServer.java:44)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:623)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:90)
java.lang.NullPointerException
at com.sun.grizzly.connectioncache.client.CacheableConnectorHandler.notifyCallbackHandlerPseudoConnect(CacheableConnectorHandler.java:222)
at com.sun.grizzly.connectioncache.client.CacheableConnectorHandler.doConnect(CacheableConnectorHandler.java:164)
at com.sun.grizzly.connectioncache.client.CacheableConnectorHandler.connect(CacheableConnectorHandler.java:117)
at SimpleTestServer.sendTest(SimpleTestServer.java:132)
at SimpleTestServer.main(SimpleTestServer.java:45)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:623)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:90)
-----
Here is the NPE's line.
In CacheableConnectorHandler.java
-----
private voide notifyCallbackHandlerPseudoConnect() throws ClosedChannelException {
...
SelectionKey key = underlyingConnectorHandler.getUnderlyingChannel().keyFor( protocolSelector );
...
}
I think that ConnectorHandler's socketChannel can be null if unexpected exceptions are occurred in CallbackHandler. ex) if an error was occurred before calling ConnectorHandler#finishConnect(SelectionKey key)
So I think that graceful handling of NPE and more check logic are needed about underlyingConnectorHandler.getUnderlyingChannel().
And I reviewed why this error had been occurred for a while.
When I inserted some test code for debugging, the following error was occurred.
-----
java.lang.ClassCastException: com.sun.grizzly.connectioncache.client.CacheableConnectorHandler cannot be cast to com.sun.grizzly.TCPConnectorHandler
at com.sun.grizzly.DefaultCallbackHandler.onConnect(DefaultCallbackHandler.java:128)
at com.sun.grizzly.connectioncache.client.CacheableConnectorHandler.onConnect(CacheableConnectorHandler.java:250)
at com.sun.grizzly.CallbackHandlerContextTask.doCall(CallbackHandlerContextTask.java:68)
at com.sun.grizzly.SelectionKeyContextTask.call(SelectionKeyContextTask.java:57)
at com.sun.grizzly.ContextTask.run(ContextTask.java:69)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
at java.lang.Thread.run(Thread.java:717)
-----
Here is DefaultCallbackHandler#onConnect(IOEvent<Context> ioEvent);
In DefaultCallbackHandler.java
-----
public void onConnect( IOEvent<Context> ioEvent ) {
SelectionKey key = ioEvent.attachment().getSelectionKey();
if( ioEvent.attachment().getProtocol() == Controller.Protocol.TCP ) {
((TCPConnectorHandler)connectorHandler).setUnderlyingChannel((SocketChannel)key.channel());
} else if( ioEvent.attachment().getProtocol() == Controller.Protocol.TLS) {
((SSLConnectorHandler)connectorHandler).setUnderlyingChannel((SocketChannel)key.channel());
}
try {
connectorHandler.finishConnect(key);
...
}
...
}
-----
I think that above calling setUnderlyingChannel() is redundant because both TCPConnectorHandler#finishConnect() and SSLConnectorHandler#finishConnect() already assign socketChannel key's channel.
Of course, it maybe has no problem if I don't use DefaultCallbackHandler.
But I hope that above reduntant code could be removed and I am curious to know if my use and point have any faults.
Please give me advice.
Thanks.
--
Bongjae Chang