Skip to content

Close the web socket after sending a close frame. #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 3, 2016

Conversation

dave-r12
Copy link
Contributor

Previously, the underlying socket was being closed with the cancel call.
Because the socket was closed, the close frame could never be sent.

Previously, the underlying socket was being closed with the cancel call.
Because the socket was closed, the close frame could never be sent.
@xuduo
Copy link

xuduo commented Jul 21, 2016

I tried this.
My app crash report still shows crash, may be less than before.
I can't reproduce this crash, only from crash report system.

@dave-r12
Copy link
Contributor Author

@xuduo this fixes one scenario where the RejectedExecutionException can happen. There may be others. If you can provide any information on your usage, please let me know.

@joshimohit
Copy link

@dave-r12 do this fix socketio/socket.io-client-java#295 also.

@nkzawa can we have it reviewed and merged. square/okhttp#2455

@dave-r12
Copy link
Contributor Author

dave-r12 commented Aug 3, 2016

@joshimohit I don't think it fixes that one. If you could provide any information on how you generate that exception, I can try to look into it.

@joshimohit
Copy link

@dave-r12 : we have been facing issues with Webscoket getting closed when socket is disconnecting and message is sent . in this case exception is received either on RealWebSocket.close() or RealWebsocket.onMessage. Generally, it happens in very low network conditions. let me know if you can help

@dave-r12
Copy link
Contributor Author

@joshimohit is the server disconnecting or is the client disconnecting? I'll have a look.

@joshimohit
Copy link

joshimohit commented Aug 11, 2016

@dave-r12 : We have observed this in very low internet bandwidth, when client- socket gets disconnected automatically. Below are steps to reproduce it :

  1. Connect android phone(Say Phone_A) to another android phone's (Phone_B) wifi hotspot. Phone_B have 2G network and Phone_A is connected to it.
  2. Now Run loop of socket.emit on Phone_A. if network bandwidth of Phone_B is too low socket will get disconnected and tries to connect again. in such scenario's socket may try to emit the message on closed socket. And alas, crash occurs.
    onMessage_error.txt
    onClose_error.txt

@joshimohit
Copy link

Hey @dave-r12 were you able to produce the bug ?

@dave-r12
Copy link
Contributor Author

I haven't looked, maybe in the next couple days.

@PiotrWpl
Copy link

@dave-r12 any update? I'm asking because it's a really big problem in our most popular app.
zrzut ekranu 2016-08-23 o 08 10 09

@xuduo
Copy link

xuduo commented Aug 23, 2016

I solved this by try catch in okhttp source code.
huangzhilong/okhttp@286ece1

@joshimohit
Copy link

@xuduo We are using Socket 0.7.0 in our Android App which inturn uses okhttp version 3.0.1. Fix you are suggesting is in okhttp v 3.4.1 ? Is socketIO Library compatible with this okhttp version (3.4.1) ? What is version of okhttp-ws you are using ?

@xuduo
Copy link

xuduo commented Aug 23, 2016

@joshimohit I use okhttp & okhttp-ws 3.4.1 with socket.io 0.7.0 , works fine. Might need some of your code if you are using https.
okhttp-ws 3.4.1 didn't fix the problem, I added try catch in my fork of okhttp, we use the forked version of okhttp.

@PiotrWpl
Copy link

@xuduo
What are defects of using try-catch in this case?

@xuduo
Copy link

xuduo commented Aug 30, 2016

Crash rate reduced, I guess this exception happens when client try to reconnect, or Android is shutting down my app.No defects have been reported.

@PiotrWpl
Copy link

@xuduo
Thx, and what with:
okhttp/src/main/java/okhttp3/RealCall.java line 83
in Yours commit?
Is this change also necessary to reduce crash rate?

@xuduo
Copy link

xuduo commented Aug 31, 2016

@PiotrWpl That's a revert to a change I made earlier which didn't work,It instead caused a stack overflow.
The other change I made was huangzhilong/okhttp@5bf24f2.
Try catch a SecurityException which cause crash on some device and throw a UnknownHostException.
link square/okhttp#2283

@dave-r12
Copy link
Contributor Author

dave-r12 commented Sep 3, 2016

Is there something I can do to get this merged in?

@nkzawa nkzawa merged commit 3f5ff6e into socketio:master Sep 3, 2016
@dave-r12
Copy link
Contributor Author

dave-r12 commented Sep 3, 2016

Thanks @nkzawa!

@nkzawa
Copy link
Contributor

nkzawa commented Sep 3, 2016

@dave-r12 Thanks for your awesome PR, and sorry for the delay! I will release the next version asap.

@joshimohit
Copy link

@nkzawa, When shall we expect next release ? onClose issue and okhttp issue
needs to be quick fixed at our end ?

On Sat, Sep 3, 2016 at 9:20 AM, Naoyuki Kanezawa notifications@github.com
wrote:

@dave-r12 https://github.com/dave-r12 Thanks for your awesome PR, and
sorry for the delay! I will release the next version asap.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants