fix added to fix following crash

[eckovation]

@nkzawa @chendrak this pull request addresses the following issue
socketio/socket.io-client-java#346

Could you please confirm if this is logical thing to do?

Fatal Exception: java.lang.IllegalStateException: closed
at okhttp3.internal.ws.RealWebSocket.sendMessage(RealWebSocket.java:107)
at io.socket.engineio.client.transports.WebSocket$4.call(WebSocket.java:189)
at io.socket.engineio.parser.Parser.encodePacket(Parser.java:63)
at io.socket.engineio.parser.Parser.encodePacket(Parser.java:42)
at io.socket.engineio.client.transports.WebSocket.write(WebSocket.java:184)
at io.socket.engineio.client.Transport$3.run(Transport.java:108)
at io.socket.thread.EventThread.exec(EventThread.java:55)
at io.socket.engineio.client.Transport.send(Transport.java:103)
at io.socket.engineio.client.Socket.flush(Socket.java:615)
at io.socket.engineio.client.Socket.onDrain(Socket.java:606)
at io.socket.engineio.client.Socket.access$1100(Socket.java:31)
at io.socket.engineio.client.Socket$6.call(Socket.java:308)
at io.socket.emitter.Emitter.emit(Emitter.java:117)
at io.socket.engineio.client.transports.WebSocket$3$1.run(WebSocket.java:171)
at io.socket.thread.EventThread$2.run(EventThread.java:80)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
at java.lang.Thread.run(Thread.java:818)

Indirect Replication Steps:
This bug happens a lot in wild. Typical reason is that, the socket is "sometimes" closed, just when the client was about to write something on it.

If you forcefully close the socket (by calling doClose()) just before the execution of this line (https://github.com/socketio/engine.io-client-java/blob/master/src/main/java/io/socket/engineio/client/transports/WebSocket.java#L189) then it will crash giving out IllegalStateException at the very next moment! But if you add this fix, it won't crash at this place.

[nkzawa]

We'd not need to call doClose again, if the socket is already closed ?

[eckovation]

@nkzawa I have now done that! removed that line.

[nkzawa]

Thanks for your PR! Added a comment.
I appreciate if you can test this PR actually fixes the issue on your app.

[eckovation]

Okay. I will remove doClose(). Apart from that what do you think about the
fix?

Is it an intelligent thing to do? To catch the illegalstateexception?

I tested the app locally it wasn't crashing. I have not yet tested this in
wild.
On Wed, 12 Oct 2016 at 9:05 AM, Naoyuki Kanezawa notifications@github.com
wrote:

Thanks for your PR! Added a comment.
I appreciate if you can test this PR actually fixes the issue on your app.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#73 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFEUGbm3zeOdqOGK1LZtFokNtsRUMj10ks5qzFV8gaJpZM4KUMzT
.

[eckovation]

Because the problem here is that, Okhttp websocket throws this
illegalstateexception. So I think we must definitely handle it.

Which is also supported from the fact that, illegalstateexception has been
causing lot of crashes lately.

Please let me know your thoughts, so the we can move forward.
On Wed, 12 Oct 2016 at 10:31 AM, Akshat Goel akshatgo@gmail.com wrote:

Okay. I will remove doClose(). Apart from that what do you think about the
fix?

Is it an intelligent thing to do? To catch the illegalstateexception?

I tested the app locally it wasn't crashing. I have not yet tested this in
wild.
On Wed, 12 Oct 2016 at 9:05 AM, Naoyuki Kanezawa notifications@github.com
wrote:

Thanks for your PR! Added a comment.
I appreciate if you can test this PR actually fixes the issue on your app.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#73 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFEUGbm3zeOdqOGK1LZtFokNtsRUMj10ks5qzFV8gaJpZM4KUMzT
.

[eckovation]

@nkzawa I have added the possible replication steps in the description of this PR. Could you please check it?

[shaacker]

Could this be a race condition?
doClose() closes the Websocket, but the ReadyState is only updated after the socket is actually closed.
During this timeframe, we could technically get past this check, then the websocket closes and then it tries to send.

@eckovation How big are the payloads you send? If they are fairly big or the phone is slow, you might run into said race condition.

[eckovation]

@chendrak
Payloads are generally small. But yes, the phone might be very slow. So yes, there is a possibility of a Race condition.
What should be the ideal way to check then?

Also, if we just catch the illegalstateexception, what are the downsides of it?
I understand, that in most cases, this problem won't even happen since, for closed sockets it won't even get past Line#179.

[shaacker]

@eckovation I think catching the IllegalStateException is the right decision here, adding a second ReadyState check before trying to send would make the code less readable.

[nkzawa]

@eckovation awesome job, thank you!!

[eckovation]

@nkzawa @chendrak thank you!

[eckovation]

@nkzawa can you release a build as well so that we can release it.

[eckovation]

@nkzawa Sorry for constant pinging. Can you release a new version of repository with the above fix?

[eckovation]

Just to sure, we are fixing this bug.

[nkzawa]

@eckovation Released, sorry to be late!