njones / socketio

A Modern SocketIO library for go
MIT License
60 stars 9 forks source link

socket-io: Closing Browser Tab and OnDisconnect Event #49

Open nmilallos97 opened 1 year ago

nmilallos97 commented 1 year ago

Upon exploring the library, I noticed that the OnDisconnect event does not trigger when closing the browser tab of an active socket connection.

However calling socket.disconnect() on client side triggers the OnDisconnect event.

njones commented 1 year ago

This library is a server implementation of the SocketIO library. It's not a client which is what will run the the browser. So closing a browser shouldn't trigger any events in this library.

Or I may be missing something. Can you point out where in the js version of the SocketIO library there is a OnDisconnect event triggered when the browser is closed, so that I can see what's done to emulate it?

nmilallos97 commented 1 year ago

I see, because I used a different go library before that has this feature, namely: https://github.com/googollee/go-socket.io. Sadly this library uses an outdated version of socket.io server which won't work with our front-end technologies.

Here's the documentation with regards to the disconnect trigger: https://socket.io/docs/v4/how-it-works/#disconnection-detection

For now I just forked the library and modified some stuff to have this mechanism work. In my case, I just modified that if the connection timed out it will trigger the onDisconnect event provided the said connection hasn't been disconnected manually by the client.

njones commented 1 year ago

Ah, I see. I missed that functionality in the documentation!

I saw your fork... I think there could be a race condition around the setting of the disconnect boolean and calling disconnect(), but I'm not positive. (in transport/transport.go:80 and transport/transport.go:84 in commit fdc1a5aa)

Would you consider adding a test around your change and submitting a Pull Request? This would be a good addition to the library. If you don't feel comfortable or have the time, please let me know, and I'll add some equivalent code.

njones commented 1 year ago

I will add this functionality in, over the next week. Thanks for reporting it!

StephenSorriaux commented 1 year ago

Hello,

Thank you for the project.

Just wanted to add that this would be a great thing to have. I did something on my side based on what @nmilallos97 already did in order to trigger the OnDisconnected when the websocket is closed (ie. when a client leaves, see https://github.com/StephenSorriaux/socketio/commit/a44d6086c0f9d25043cb23d80302c5e68b385ad9). It is probably not the best way to do it (only added for websocket transport, and for the v4 server for instance) but just wanted to share in case you have time for some comments.

laagland commented 1 year ago

I will add this functionality in, over the next week. Thanks for reporting it!

Hi @njones, is this something that you're still planning to include? Would really like to have this functionality.

njones commented 8 months ago

Propagate the Disconnect() event through the session within the transport.go function.

Projected completion: Jan. 29, 2024