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

Type error on error closing connection #1591

Closed
grimkirill opened this issue Jun 17, 2019 · 14 comments
Closed

Type error on error closing connection #1591

grimkirill opened this issue Jun 17, 2019 · 14 comments

Comments

@grimkirill
Copy link

@grimkirill grimkirill commented Jun 17, 2019

  • I've searched for any related issues and avoided creating a duplicate
    issue.

Description

Type error:

node_modules/ws/lib/websocket.js:900
websocket.readyState = WebSocket.CLOSING;
^

TypeError: Cannot set property 'readyState' of undefined

Reproducible in:

  • version: ^7.0.0
  • Node.js version(s): 8
  • OS version(s): ubuntu 18

Steps to reproduce:

  1. Connect

  2. Disconnect by broken connection

Expected result:

Actual result:

Attachments:

@lpinca
Copy link
Member

@lpinca lpinca commented Jun 17, 2019

Are you able to create a reproducible test case? This can only happen if the 'error' event is emitted after the 'close' event on the net.Socket but it would be a bug in Node.js.

@grimkirill
Copy link
Author

@grimkirill grimkirill commented Jun 17, 2019

It was easy reproduced on production load.
v6.1.4 has additional check, that allow pass this error

if (websocket) {
    websocket.readyState = WebSocket.CLOSING;
    this.destroy();
  }
@lpinca
Copy link
Member

@lpinca lpinca commented Jun 18, 2019

Yes, it was originally needed due to this issue: #1226.

We fixed it in #1464 and #1471, and removed the check in 297f56d.

We can add it back but it would be nice to know how this can happen so we can also add a regression test.

@grimkirill
Copy link
Author

@grimkirill grimkirill commented Jun 18, 2019

It was happening at the high load. nodejs use near of 90% cpu usage, socket with tls and part of connections was broken by network issues.
Maybe some of combination of: termination + timeout + error

@grimkirill
Copy link
Author

@grimkirill grimkirill commented Jun 18, 2019

i think that found problem: missed remove error handler, that produced race

function socketOnClose() {
  ...
  this.removeListener('data', socketOnData);
  this.removeListener('error', socketOnError); // missed
  this[kWebSocket] = undefined;
@lpinca
Copy link
Member

@lpinca lpinca commented Jun 18, 2019

No, if we do that and an 'error' event emitted after the 'close' event, it will crash the process.

@grimkirill
Copy link
Author

@grimkirill grimkirill commented Jun 18, 2019

Yep you are right.
So only one way is the return of websocket check undefined, because error event can be fire after socketOnClose execution

@lpinca
Copy link
Member

@lpinca lpinca commented Jun 18, 2019

Yes I will revert 297f56d.

@lpinca
Copy link
Member

@lpinca lpinca commented Jun 18, 2019

@grimkirill if you can reproduce this, can you try with Node.js 10+? I would like to see if the issue is limited to Node.js 8.

Thank you.

@lpinca
Copy link
Member

@lpinca lpinca commented Jul 2, 2019

@grimkirill any update?

@chuahcheeshian
Copy link

@chuahcheeshian chuahcheeshian commented Jul 4, 2019

@lpinca I'm using Node.js 12, also having this issue as well.

@lpinca
Copy link
Member

@lpinca lpinca commented Jul 4, 2019

@chuahcheeshian same stack trace? Can you post it? Thank you.

@chuahcheeshian
Copy link

@chuahcheeshian chuahcheeshian commented Jul 5, 2019

Screen Shot 2019-07-05 at 9 52 24 AM

@lpinca

On my staging servers with little to no load, this error never happened at all.
On my production servers, this error only happened randomly under high load.
With 2k concurrent WebSocket connections, this error happened at least 8 times/hour.

And when it happened, the server crashed and got restarted by Docker intentionally.
And then all the previously connected clients reconnected at once.
Maybe under high load, this error might stand a better chance to be triggered?
Or should there be any bug in my code that led to this error?

I had then downgraded ws to 6.2.0 in which the if (websocket) clause is present.
My production servers have never been restarted since then.

For more information, the version of Node.js I'm using is 12.6.0, the version of ws is 7.0.1.

@lpinca
Copy link
Member

@lpinca lpinca commented Jul 5, 2019

Ok I've reverted 297f56d. Will cut a new release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants