search

sacOO7 / socketcluster-client-java

Native java and android client for socketcluster framework in node.js
http://socketcluster.io/
Apache License 2.0
94 stars 42 forks source link

Refactoring on reconnect strategy #26

Closed XDex closed 6 years ago

XDex commented 6 years ago

When the SC server doesn't receive a PONG response on time, it disconnects the SC client. In this case onDisconnected callback is not called, but instead onCloseFrame callback gets called, and this latter one doesn't contain any reconnect logic. This PR fixes this issue, along with a few minor code improvements.

XDex commented 6 years ago

@sacOO7 Could you please take a quick look, merge it if everything is ok and publish a new client release?

sacOO7 commented 6 years ago

Hi @XDex , sure, will take a look at PR and get back to you 👍

sacOO7 commented 6 years ago

Hi @XDex , just one question. Is onDisconnected not getting called when server closes the connection? I think when server closes connection, it will call both onFrameClose and onDisconnected, and we will handle just onDisconnected event. There might have been change in protocol from server side. I'm not sure :P. Can you check it please? Thank you for the PR :+1:

XDex commented 6 years ago

@sacOO7 As I stated in the first comment onDisconnected doesn't get called, and only onCloseFrame does. You can repro this by commenting out the line which responds to a PING (websocket.sendText("#2");), and then waiting till the server disconnects the client due to ping timeout..

sacOO7 commented 6 years ago

Hi @XDex , sorry but I was not able to reproduce the issue. On my local environment, I am able to get disconnect event if server closes the connection. It's just I said earlier, it will call onCloseFrame and then it will close the connection (Will call onDisconnect). Can you please post here your node version and socketcluster version? That way I will try to reproduce this issue again. If I merge this PR now, it might create double reconnect function calls ( One by ondisconnect and one by onCloseFrame). Please try to install latest sc and try to reproduce again yourself

sacOO7 commented 6 years ago

Hi @XDex , any updates on this?

XDex commented 6 years ago

@sacOO7 Unfortunately, I haven't been able to reproduce it since I last observed it :( This probably means that the issue was caused by something else. Basically, the client was on a slow network connection, and the Android app was doing some heavy processing on the main thread. The last message logged on the client was "On close frame got called", but without any disconnection message and not followed by any reconnection attempts. And on the server it was logged that the client was disconnected due to ping timeout.. I will try to reproduce it and will open another issue if I manage it.. For now I guess just the refactorings can be merged, if they look ok to you (I have removed the reconnect() call inside onCloseFrame() callback)

sacOO7 commented 6 years ago

Hi @XDex , I am merging this PR for now, please check if you find any other issues :+1: