Strange error handling in Grizzly 2.3.4 when connection refused?

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Strange error handling in Grizzly 2.3.4 when connection refused?

Matthew Swift-3
Hi there,

While trying to debug one of our unit tests which is failing due to a client connection attempt being refused I came across a surprising looking piece of code. The stack trace is (latest 2.3.4 nightly):

at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler.abortConnection(TCPNIOConnectorHandler.java:262)
at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler.onConnectedAsync(TCPNIOConnectorHandler.java:215)
at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler$1.completed(TCPNIOConnectorHandler.java:154)
at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler$1.completed(TCPNIOConnectorHandler.java:150)
at org.glassfish.grizzly.nio.transport.TCPNIOConnection.onConnect(TCPNIOConnection.java:258)
at org.glassfish.grizzly.nio.transport.TCPNIOTransport.fireIOEvent(TCPNIOTransport.java:826)
at org.glassfish.grizzly.strategies.AbstractIOStrategy.fireIOEvent(AbstractIOStrategy.java:113)
at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.run0(WorkerThreadIOStrategy.java:115)
at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.executeIoEvent(WorkerThreadIOStrategy.java:101)
at org.glassfish.grizzly.strategies.AbstractIOStrategy.executeIoEvent(AbstractIOStrategy.java:89)
at org.glassfish.grizzly.nio.SelectorRunner.iterateKeyEvents(SelectorRunner.java:406)
at org.glassfish.grizzly.nio.SelectorRunner.iterateKeys(SelectorRunner.java:375)
at org.glassfish.grizzly.nio.SelectorRunner.doSelect(SelectorRunner.java:339)
at org.glassfish.grizzly.nio.SelectorRunner.run(SelectorRunner.java:271)
at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:565)
at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:545)
at java.lang.Thread.run(Thread.java:662)
Caused by: java.net.ConnectException: Connection refused
at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
at sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:599)
at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler.onConnectedAsync(TCPNIOConnectorHandler.java:205)
... 15 more


On closer examination of the method TCPNIOConnectorHandler.onConnectedAsync(...) I see this:

    protected static void onConnectedAsync(final TCPNIOConnection connection,
            final CompletionHandler<Connection> completionHandler) {

        final TCPNIOTransport tcpTransport =
                (TCPNIOTransport) connection.getTransport();
        final SocketChannel channel = (SocketChannel) connection.getChannel();
        
        try {
            if (!channel.isConnected()) {
                channel.finishConnect();
            }

            connection.resetProperties();

            // Deregister OP_CONNECT interest
            connection.disableIOEvent(IOEvent.CLIENT_CONNECTED);

            tcpTransport.configureChannel(channel);
        } catch (Exception e) {
            abortConnection(connection, completionHandler, e);
            throw new IllegalStateException(e);
        }
        
        if (connection.notifyReady()) {
            tcpTransport.fireIOEvent(IOEvent.CONNECTED, connection,
                    new EnableReadHandler(completionHandler));
        }
    }

The call to finishConnect is failing with a ConnectException, yet the exception handling code is treating it as if it is unexpected by throwing an IllegalStateException. The current behavior is benign I think since the runtime exception is trapped further up the stack in TCPNIOConnection.onConnect(). However, it's pretty fragile IMO since callers may not be aware that this method may throw a runtime exception for an expected error condition. Once the connection is aborted, does an exception need to be re-thrown at all?

Cheers,

Matt

Reply | Threaded
Open this post in threaded view
|

Re: Strange error handling in Grizzly 2.3.4 when connection refused?

oleksiys
Administrator
Hi Matt,

agree it's a design issue.
CompletionHandler doesn't fit well, we need something that allows IOException to be thrown. I'll rework that.
Thank you for bringing this up.

WBR,
Alexey.

On 02.07.13 06:54, Matthew Swift wrote:
Hi there,

While trying to debug one of our unit tests which is failing due to a client connection attempt being refused I came across a surprising looking piece of code. The stack trace is (latest 2.3.4 nightly):

at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler.abortConnection(TCPNIOConnectorHandler.java:262)
at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler.onConnectedAsync(TCPNIOConnectorHandler.java:215)
at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler$1.completed(TCPNIOConnectorHandler.java:154)
at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler$1.completed(TCPNIOConnectorHandler.java:150)
at org.glassfish.grizzly.nio.transport.TCPNIOConnection.onConnect(TCPNIOConnection.java:258)
at org.glassfish.grizzly.nio.transport.TCPNIOTransport.fireIOEvent(TCPNIOTransport.java:826)
at org.glassfish.grizzly.strategies.AbstractIOStrategy.fireIOEvent(AbstractIOStrategy.java:113)
at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.run0(WorkerThreadIOStrategy.java:115)
at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.executeIoEvent(WorkerThreadIOStrategy.java:101)
at org.glassfish.grizzly.strategies.AbstractIOStrategy.executeIoEvent(AbstractIOStrategy.java:89)
at org.glassfish.grizzly.nio.SelectorRunner.iterateKeyEvents(SelectorRunner.java:406)
at org.glassfish.grizzly.nio.SelectorRunner.iterateKeys(SelectorRunner.java:375)
at org.glassfish.grizzly.nio.SelectorRunner.doSelect(SelectorRunner.java:339)
at org.glassfish.grizzly.nio.SelectorRunner.run(SelectorRunner.java:271)
at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:565)
at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:545)
at java.lang.Thread.run(Thread.java:662)
Caused by: java.net.ConnectException: Connection refused
at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
at sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:599)
at org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler.onConnectedAsync(TCPNIOConnectorHandler.java:205)
... 15 more


On closer examination of the method TCPNIOConnectorHandler.onConnectedAsync(...) I see this:

    protected static void onConnectedAsync(final TCPNIOConnection connection,
            final CompletionHandler<Connection> completionHandler) {

        final TCPNIOTransport tcpTransport =
                (TCPNIOTransport) connection.getTransport();
        final SocketChannel channel = (SocketChannel) connection.getChannel();
        
        try {
            if (!channel.isConnected()) {
                channel.finishConnect();
            }

            connection.resetProperties();

            // Deregister OP_CONNECT interest
            connection.disableIOEvent(IOEvent.CLIENT_CONNECTED);

            tcpTransport.configureChannel(channel);
        } catch (Exception e) {
            abortConnection(connection, completionHandler, e);
            throw new IllegalStateException(e);
        }
        
        if (connection.notifyReady()) {
            tcpTransport.fireIOEvent(IOEvent.CONNECTED, connection,
                    new EnableReadHandler(completionHandler));
        }
    }

The call to finishConnect is failing with a ConnectException, yet the exception handling code is treating it as if it is unexpected by throwing an IllegalStateException. The current behavior is benign I think since the runtime exception is trapped further up the stack in TCPNIOConnection.onConnect(). However, it's pretty fragile IMO since callers may not be aware that this method may throw a runtime exception for an expected error condition. Once the connection is aborted, does an exception need to be re-thrown at all?

Cheers,

Matt