crossbario / autobahn-java

WebSocket & WAMP in Java for Android and Java 8
https://crossbar.io/autobahn
MIT License
1.52k stars 426 forks source link

NettyWebSocket leaks it's NioEventLoopGroup resources on connection failure #433

Closed jellisgwn closed 5 years ago

jellisgwn commented 5 years ago

If you attempt to connect to a crossbar that isn't listening, or fail to connect for another reason, the NettyWebSocket leaks socket / stream resources associated with the NioEventLoopGroup. If the application is persistently attempting reconnection, and the issue is not resolved, the process will eventually run out of file handles.

As each call to NettyWebSocket creates a new NioEventLoopGroup, and doesn't keep a reference to it outside of connect() it never gets closed / shutdown.

jellisgwn commented 5 years ago

I'm not at all sure if this is an appropriate fix, but it's what worked...

$ git diff
diff --git a/autobahn/src/main/java/io/crossbar/autobahn/wamp/Client.java b/autobahn/src/main/java/io/crossbar/autobahn/wamp/Client.java
index dede831..05b1512 100644
--- a/autobahn/src/main/java/io/crossbar/autobahn/wamp/Client.java
+++ b/autobahn/src/main/java/io/crossbar/autobahn/wamp/Client.java
@@ -121,8 +121,14 @@ public class Client {
                 mSession.join(mRealm, mAuthenticators).thenAccept(details ->
                         LOGGER.i(String.format("JOINED session=%s realm=%s", details.sessionID,
                                 details.realm))));
-        mSession.addOnDisconnectListener((session, wasClean) ->
-                exitFuture.complete(new ExitInfo(wasClean)));
+        mSession.addOnDisconnectListener((session, wasClean) -> {
+                try {
+                    mTransports.get(0).close();
+                } catch (Exception f) {
+                    // no idea what to do with this
+                }
+                exitFuture.complete(new ExitInfo(wasClean));
+       });
         CompletableFuture.runAsync(() -> {
             try {
                 mTransports.get(0).connect(mSession, options);
diff --git a/autobahn/src/main/java/io/crossbar/autobahn/wamp/transports/NettyWebSocket.java b/autobahn/src/main/java/io/crossbar/autobahn/wamp/transports/NettyWebSocket.java
index 1a5fd35..49b3da7 100644
--- a/autobahn/src/main/java/io/crossbar/autobahn/wamp/transports/NettyWebSocket.java
+++ b/autobahn/src/main/java/io/crossbar/autobahn/wamp/transports/NettyWebSocket.java
@@ -66,6 +66,8 @@ public class NettyWebSocket implements ITransport {
     private NettyWebSocketClientHandler mHandler;
     private final String mUri;

+    private EventLoopGroup mGroup;
+
     private WebSocketOptions mOptions;
     private String mSerializers;

@@ -157,9 +159,9 @@ public class NettyWebSocket implements ITransport {
                 new DefaultHttpHeaders(), options.getMaxFramePayloadSize());
         mHandler = new NettyWebSocketClientHandler(handshaker, this, transportHandler);

-        EventLoopGroup group = new NioEventLoopGroup();
+        mGroup = new NioEventLoopGroup();
         Bootstrap bootstrap = new Bootstrap();
-        bootstrap.group(group);
+        bootstrap.group(mGroup);
         bootstrap.channel(NioSocketChannel.class);

         TransportOptions opt = options;
@@ -210,6 +212,9 @@ public class NettyWebSocket implements ITransport {
     @Override
     public void close() throws Exception {
         LOGGER.v("close()");
+       if (mGroup != null) {
+           mGroup.shutdownGracefully().sync();
+        }
         if (mHandler != null && mChannel != null) {
             mHandler.close(mChannel, true, new CloseDetails(CloseDetails.REASON_DEFAULT, null));
         }

Unless i've misunderstood the flow here, it seems that these resources would be lost on any disconnect, not just the case in which i'm encountering it.

jellisgwn commented 5 years ago

@om26er / @oberstet can someone take a look at this? despite the changes above resolving the issue for me locally, i can't shake the feeling that something more fundamental is missing in the connection handling... but i don't have a design in my head to work towards the right solution.

jellisgwn commented 5 years ago

@oberstet has development stopped on autobahn-java? I've not seen any announcements, but it would be useful to know if this is project is not going to continue to move forward...

om26er commented 5 years ago

@jellisgwn Hey! the project is active and we do have plans for it going forwards as well. Recently we have been busy with other stuff.

I'll look into this issue...

oberstet commented 5 years ago

sure, this project continues .. currently active development has shifted to adding support for XBR (https://xbr.network/) to ABJ - but shaking out bugs like this too. @om26er could you have a look? ah - you beat me;) 13s quicker

jellisgwn commented 5 years ago

@om26er, @oberstet that's great to hear! I was not looking forward to having to find another solution :)