Atmosphere / wasync

WebSockets with fallback transports client library for Node.js, Android and Java
http://async-io.org
161 stars 47 forks source link

Reconnect issue after server restart #133

Closed manikantag closed 8 years ago

manikantag commented 9 years ago

Hi,

I m trying to use wasync client to establish Websocket connection to another server. And like any production app, we need it to be resilient to server restarts and once server is up, client should reconnect automatically. In one line, we want always-on connection to server.

I've written typical program, but still I m facing below issues:

  1. Once I enable reconnect(true) option, client is reconnecting to server for the first time, but REOPEN event handler function is NOT being called. But STATUS handler is called with 101 response.
  2. If server down again, once the server is up, client is NOT reconnecting again.

You can find the project here: https://github.com/manikantag/AtmosphereTest. Log file: https://github.com/manikantag/AtmosphereTest/blob/master/log.txt

I've checked multiple times and multiple places, but I m getting if I m doing something wrong. I've also checked the pull request (https://github.com/Atmosphere/wasync/pull/101) for this very issue.

jfarcand commented 9 years ago

@manikantag which version and which JDK are you using?

manikantag commented 9 years ago

Using all latest versions. The project I've mentioned has the pom having all these versions except Tomcat.

JDK 1.8 wasync: 2.1.2 async-http-client: 1.9.31 (netty 4.0.31.Final) Atmosphere: 2.3.4 Jersey: 1.19 Tomcat: 8.0.20.0

Thanks.

jfarcand commented 9 years ago

@manikantag Netty version is wrong for sure :-) Should be 3.x :-)

manikantag commented 9 years ago

Hi, Still facing same issue even after using Netty 3.10.4.Final. I've verified that Netty 4.x jars are NOT present in WEB-INF/lib.

I m not seeing any difference even in the trace.

I've updated the project pom

manikantag commented 9 years ago

@jfarcand Did you able to look into this? Thanks.

manikantag commented 9 years ago

I debugged the issue further. I think I have something about issue '2' I mentioned above.

When the server is stopped for first time, org.atmosphere.wasync.transport.WebSocketTransport.close() is calling com.ning.http.client.providers.netty.ws.NettyWebSocket.close(), which is clearing all the listeners.

@Override
public void close() {
    if (channel.isOpen()) {
        onClose();
        listeners.clear(); // <----------- Clearing the listeners
        channel.write(new CloseWebSocketFrame()).addListener(ChannelFutureListener.CLOSE);
    }
}

But com.ning.http.client.providers.netty.ws.NettyWebSocket.onClose(int, String) calling l.onClose(...) only if some listeners present, and due to this, I think, CLOSE is not firing from then on (i.e., CLOSE event or the re-connection is not happening after first time the server was shutdown).

Quoting from com.ning.http.client.providers.netty.ws.NettyWebSocket.onClose():

public void onClose(int code, String reason) {
    for (WebSocketListener l : listeners) {
        try {
            if (l instanceof WebSocketCloseCodeReasonListener) {
                WebSocketCloseCodeReasonListener.class.cast(l).onClose(this, code, reason);
            }
            l.onClose(this);
        } catch (Throwable t) {
            l.onError(t);
        }
    }
}

Is this the expected behavior? If yes, do I have to add listener on closing?

slandelle commented 9 years ago

@jfarcand FYI, I checked that this isn't a regression I introduced in AHC1.9, things worked already this way in 1.8.

IMO, as the reconnect logic is in wasync and not in AHC, it's wasync's responsibility to set again the proper listeners, as AHC definitively wants to clean everything and don't risk leaking the listeners. The other possibility is to contribute the reconnect logic into AHC :)

manikantag commented 9 years ago

So, this is expected behavior as per AHC then. But, wasync should set the listeners again after re-connection. I'll see if I can debug further. Thanks @slandelle

jfarcand commented 9 years ago

@manikantag Your contribution is welcomed as I don't have the cycle to work on this for the next 2 weeks (will do my best to look at it). But the explanation from @slandelle makes it easy to fix, e.g we just need to re-add the listener.

NKame commented 8 years ago

I've added

            // listeners were previously removed by calling webSocket.close()
            WebSocketListener l = new TextListener();
            if (supportBinary) {
                l = new BinaryListener(l);
            }
            webSocket.addWebSocketListener(l);

in WebSocketTransport::reconnect. Now the reconnection works, but the first message received on the WebSocket, which looks like atmo id|0|X does not seem to be handled. Maybe TrackMessageSizeDecoder needs to be reinitialized on reconnection? Or on some kind of reconnections?