clearTimeout should be called from the same scope as setTimeout #1568
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Details of change: - engine.io-client sets clearTimeoutFn and setTimeoutFn function depending on settings passed to manager - socker.io-client is using manager.setTimeoutFn to start connection monitoring timer, but is using regular clearTimeout function to stop it when connection is established - In some setups it is causing timer fail to stop and it will break connection every _timeout_ milliseconds (which is 20000 by default)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The kind of change this PR does introduce
Current behaviour
In nw.js environment, when socket.io-client is imported as ES module (supposed to run in node.js context of nw.js) I observe following unexpected behavior.
Client successfully establishes connection via WebSocket transport, messages can be exchanged well, ping/pong mechanism is working well, but after 20 seconds connection is getting dropped and re-established again. It happens every 20 seconds exactly.
When I change timeout option of manager to some other value, I see that connection is getting dropped after exactly this value. That made me think that monitoring timer is not getting stopped when connection is successfully established.
I found that nw.js has several JS contexts running and problem disappears when I enable mixed context mode of nw.js (https://docs.nwjs.io/en/latest/For%20Users/Advanced/JavaScript%20Contexts%20in%20NW.js/#mixed-context-mode)
After looking into source code, I found that socket.io-client is starting timer using manager.setTimeoutFn function which is set by engine.io-client. But timer is getting stopped by regular clearTimeout function from current scope.
When I change it to manager.clearTimeoutFn problem disappears.
It seems somehow timer is starting from one scope and is getting stopped by different scope, that's why it's not actually stopped.
When I import socket.io-client as html script (running in browser context), I don't see behaviour and it works as expected.
New behaviour
Using manager.clearTimeoutFn to stop monitoring timer.
Other information (e.g. related issues)
https://docs.nwjs.io/en/latest/For%20Users/Advanced/JavaScript%20Contexts%20in%20NW.js/