go-socket.io
go-socket.io copied to clipboard
googollee
[BUG] Never calls event "OnDisconnect" after client loses a websocket connection
When my js client loses a websocket connection to my socket.io server it never calls event "OnDisconnect". I think it is bug. I only get event "OnError" with the text "websocket: close 1001 (going away)".
@svksergey Hi, Sergey. I caught next messages:
closed bye
meet error: websocket: close 1001 (going away)
I tested on default version in master branch. I think it is not correctly, because this situation is correctly and onError should don't work.
On websocket disconnection https://github.com/googollee/go-socket.io/blob/master/parser/decoder.go#L58 NextReader returns github.com/gorilla/websocket.CloseError with code 1001 websocket.CloseGoingAway.
I think wrapper.NextReader() in go-engine.io/transport/websocket/wrapper.go should check if error is websocket.CloseError and return this error wrapped with another error defined in go-engine.io API. Finally conn.serveError() https://github.com/googollee/go-socket.io/blob/master/conn.go#L196 should call onDisconnect in case of wrapped CloseError.
Edit:
Even better way: in case of wrapped CloseError decoder.NextReader() may set header.Type to parser.Disconnect and return no error (nil).
@mrfoe7 I haven't made MR yet. There are clues for solution only. I'll try to find some time in next week to implement such change.
I think there's one more change should be done even though it breaks backward compatibility. OnError handler function should have Conn type parameter just like others handlers.
Just one more question: in polling mode could we detect if Socket.IO connection is closed? I think there's no such thing, because we don't know what is going between requests from client side. From the other side: it's possible to implement some disconnection logic based on ping...
I asking because I don't know if there is similar problem with no OnDisconnect call in case of polling transport.
I'll try to find some time in next week to implement such change.
I hope to it ^^
I think there's one more change should be done even though it breaks backward compatibility. OnError handler function should have Conn type parameter just like others handlers.
It is done by PR #274 by @ardingchen and PR #237 If it is what you mean and new PR is correct, i will approve PR #274
I asking because I don't know if there is similar problem with no OnDisconnect call in case of polling transport.
I will try generate this situation on my PC and tell more information
other way
server.OnDisconnect("", func(s socketio.Conn, msg string) {
fmt.Printf("client id %s say goodbye", s.ID())
})
namespace = "" can catch client disconnect event
@mrfoe7 #274 looks good for me. After that every handlers will be called with connection context.