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

Prevent beat timeout from disconnecting newly connecting sockets #304

Merged
merged 1 commit into from
Mar 17, 2020

Conversation

rankida
Copy link
Contributor

@rankida rankida commented Mar 16, 2020

Issue

Occasionally client.connect() errors with Request failed - server disconnected milliseconds after initiating the connection.

Root cause

This error occurs because for a short lived time a socket is established on the server but the nes "hello" handshake message has not been processed so socket._active() will return false due to the fact that this._pinged is false. Once the "hello" message is processed the socket is correctly identified as active.

If the beat timeout occurs between the websocket being added to to the sockets collection but before the "hello" is processed the beat timeout logic will disconnect the socket.

Proposed fix

The proposed fix is to allow a grace period in which newly created sockets are not disconnected by the beat timeout.

Testing

Being a race condition it is difficult to test but the unit test added will fail repeatedly if my logic is removed.

@rankida rankida force-pushed the disconnected-sockets branch from 2828769 to d326f21 Compare March 16, 2020 15:14
@rankida rankida force-pushed the disconnected-sockets branch from d326f21 to 9cf67d7 Compare March 16, 2020 15:41
@rankida
Copy link
Contributor Author

rankida commented Mar 16, 2020

I don't believe the CI unit test failures are related to my change.
Linux Node 12 and node tests passing as is Window's node run.

@hueniverse hueniverse self-assigned this Mar 17, 2020
@hueniverse hueniverse added the bug Bug or defect label Mar 17, 2020
@hueniverse hueniverse added this to the 12.0.1 milestone Mar 17, 2020
@hueniverse hueniverse merged commit c24a3fc into hapijs:master Mar 17, 2020
@hueniverse hueniverse modified the milestones: 12.0.1, 12.0.2 Mar 17, 2020
hueniverse added a commit that referenced this pull request Mar 17, 2020
@rankida rankida deleted the disconnected-sockets branch March 19, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants