Skip to content
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

"close" event is not sent on Socket, if it is attached before the Socket upgrade #10

Closed
koush opened this issue Feb 26, 2019 · 3 comments

Comments

@koush
Copy link
Contributor

koush commented Feb 26, 2019

See here:
https://github.com/socketio/engine.io-server-java/blob/master/engine.io-server/src/main/java/io/socket/engineio/server/EngineIoSocket.java#L135

When the transport upgrades from XHR to WebSocket, all existing close events get cleared out. I am not sure if this is intended behavior, and I am currently working around it by reattaching close events upon receiving an upgrade event.

Repro

  1. wait for "connection" event for EngineIOSocket.
  2. Attach "close" event immediately on EngineIOSocket
  3. Wait for EngineIOSocket client transport to upgrade. Should happen more or less immediately if websocket transport is configured.
  4. Close browser or java engine io client
  5. Observe that the "close" originally attached close event is not fired. It is not even in the internal callback list, because it was cleared during upgrade.
@trinopoty
Copy link
Collaborator

That is by design. The callbacks for the old transport is cleared and new ones are registered by callingsetTransport.
I'll take a better look at it when I get the time.

@trinopoty
Copy link
Collaborator

It seems I read the original js sources a bit incorrectly.
This issue should now be fixed.
Can you please check it now.

@koush
Copy link
Contributor Author

koush commented Feb 26, 2019

Yes, this seems to work fine. Great work on the library btw. I managed to port this to Android in day, onto a custom http+websocket server stack, with zero changes. This was the only bug I encountered.

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

No branches or pull requests

2 participants