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

Cannot set property 'readyState' of undefined #1564

Closed
mccoysc opened this issue May 12, 2019 · 33 comments
Closed

Cannot set property 'readyState' of undefined #1564

mccoysc opened this issue May 12, 2019 · 33 comments

Comments

@mccoysc
Copy link

mccoysc commented May 12, 2019

websocket.readyState = WebSocket.CLOSING;
^

TypeError: Cannot set property 'readyState' of undefined
at TLSSocket.socketOnClose (/node_modules/ws/lib/websocket.js:826:24)
at TLSSocket.emit (events.js:187:15)
at _handle.close (net.js:606:12)
at Socket.done (_tls_wrap.js:386:7)
at Object.onceWrapper (events.js:273:13)
at Socket.emit (events.js:182:13)
at TCP._handle.close (net.js:606:12)

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

@lpinca i found that as a server:
if 2 or more clients(some app run in browser) are connected to this server,and if one client closed,the error would be occoured in server side.

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

https://careeron.ecspool.com
you can browse this url in 2 browser tab.
refresh one tab,the another tab's websocket connection would be in "CLOSED" state(in fact,the server side is errored at this moment)

@lpinca
Copy link
Member

lpinca commented May 12, 2019

I can't see how this can happen:

  1. The socketOnClose() listener is added by WebSocket#setSocket() after socket[kWebSocket] = this;
  2. How can websocket be undefined when the listener is called?

Please create a minimal test case, using only ws, to reproduce the issue.

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

i found that for one same websocket,the socketOnClose function is called twice:for the first,you have make "this[kWebSocket]=undefined",and so the seconed,this[kWebSocket] is undefined

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

@lpinca

@lpinca
Copy link
Member

lpinca commented May 12, 2019

If the 'close' event is emitted more than once on the net.Socket or tls.Socket then there is a bug in Node.js.

Also the listener removes itself after it is called. See https://github.com/websockets/ws/blob/6.2.1/lib/websocket.js#L823

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

but how can i resolve this problem?

@lpinca
Copy link
Member

lpinca commented May 12, 2019

I don't know. Show me where the bug is in ws and how to reproduce it then maybe I can help. I have no clue what your code is doing.

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019 via email

@lpinca
Copy link
Member

lpinca commented May 12, 2019

I cannot make it crash and that's why I'm asking you to write a test case to reproduce the issue. This is what I tried with the info you gave me and it works as expected, no exception is thrown.

const WebSocket = require('.');
const { createServer } = require('https');
const { readFileSync } = require('fs');

const server = createServer({
  cert: readFileSync('test/fixtures/certificate.pem'),
  key: readFileSync('test/fixtures/key.pem')
});
const wss = new WebSocket.Server({ server });

wss.on('connection', function(ws, { url }) {
  const params = new URLSearchParams(url.slice(url.indexOf('?')));

  ws.on('close', function() {
    console.log('closed connection %s', params.get('id'));
  });
});

server.listen(8080, function() {
  const options = { rejectUnauthorized: false };
  const ws1 = new WebSocket('wss://localhost:8080/?id=1', options);
  const ws2 = new WebSocket('wss://localhost:8080/?id=2', options);

  ws1.on('open', function() {
    ws1.close();
  });

  ws2.on('open', function() {
    ws2.close();
  });
});

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

careerOnDapp.zip

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

you can run:
node ./bin/www

and browse at
https://127.0.0.1
in 2 tabs.
and refresh one tab,
the program would be crashed

@lpinca
Copy link
Member

lpinca commented May 12, 2019

It does not crash on my machine.

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

you browser url https://127.0.0.1
and then browse the same url in another tab.

and then you refresh one tab

@lpinca
Copy link
Member

lpinca commented May 12, 2019

yes, also this is not a good test case. It's a whole app can't even see where ws is used. Where is the WebSocket server created?

$ grep --exclude-dir=node_modules -R -E "require\('ws" .

ws is not used anywhere.

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019

in "node_modules/gun/lib/ws.js"

@lpinca
Copy link
Member

lpinca commented May 12, 2019

I don't have time to debug other people code, sorry. I have no clue what gun does with ws and how it uses it. Report the issue on gun repository.

Btw I tried again and it does not crash on my machine. I've opened 3 tabs and refreshed all of them.

@lpinca
Copy link
Member

lpinca commented May 12, 2019

What Node.js version are you using? What OS?

@mccoysc
Copy link
Author

mccoysc commented May 12, 2019 via email

@lpinca
Copy link
Member

lpinca commented May 13, 2019

Closing as I can't reproduce it.

@lpinca lpinca closed this as completed May 13, 2019
@nooop3
Copy link

nooop3 commented Jun 28, 2019

may be emit 'close' multi times.
Node.JS removelistener

@orgads
Copy link

orgads commented Jul 17, 2019

We hit a similar exception when 'error' event is received after 'close'. It happens if I have an open websocket, and I'm profiling with Chrome. When I click "Stop recording heap profile", it pauses the application for a few seconds, and this leads to a crash (sometimes).

I don't have a minimal example that reproduces it yet, but I'll try to reproduce later today.

Trace:

socketOnError (f:\Project\node_modules\ws\lib\websocket.js:904)
emit (events.js:198)
EventEmitter.emit (domain.js:448)
errorOrDestroy (destroy.js:107)
onwriteError (_stream_writable.js:430)
onwrite (_stream_writable.js:461)
doWrite (_stream_writable.js:411)
clearBuffer (_stream_writable.js:522)
Writable.uncork (_stream_writable.js:319)
sendFrame (f:\Project\node_modules\ws\lib\sender.js:353)
perMessageDeflate.compress (f:\Project\node_modules\ws\lib\sender.js:311)
_compress (f:\Project\node_modules\ws\lib\permessage-deflate.js:315)
_deflate.flush (f:\Project\node_modules\ws\lib\permessage-deflate.js:449)
afterWrite (_stream_writable.js:485)
onwrite (_stream_writable.js:476)
afterTransform (_stream_transform.js:94)
processCallback (zlib.js:548)
[ ZLIB ]
init (inspector_async_hook.js:27)
emitInitNative (async_hooks.js:137)

@lpinca
Copy link
Member

lpinca commented Jul 17, 2019

@orgads see #1591

@orgads
Copy link

orgads commented Jul 19, 2019

Thank you. That was it.

@raifemre
Copy link

I introduced the same problem and found out that other libraries somehow breaks the connection on express server. On my case it was 'express-status-monitor'.

@wenzhiwang
Copy link

I introduced the same problem and found out that other libraries somehow breaks the connection on express server. On my case it was 'express-status-monitor'.

I ran into the same problem yesterday when I played with the Hackathon Starter. The "express-status-monitor" breaks the connection, indeed.

@mredbishop
Copy link

Why is this closed, regardless of why the event is being fired twice it shouldn't crash the entire app. If it's possible for the websocket not to be defined then the method should account for it. In this case it most likely should just return early.
image

@lpinca
Copy link
Member

lpinca commented Aug 28, 2020

Why is this closed, regardless of why the event is being fired twice it shouldn't crash the entire app. If it's possible for the websocket not to be defined then the method should account for it. In this case it most likely should just return early.

No, because if we do it we mask a bug instead of fixing it. The websocket cannot be undefined in that handler. If it is there is bug.

@mredbishop
Copy link

Why is this closed, regardless of why the event is being fired twice it shouldn't crash the entire app. If it's possible for the websocket not to be defined then the method should account for it. In this case it most likely should just return early.

No, because if we do it we mask a bug instead of fixing it. The websocket cannot be undefined in that handler. If it is there is bug.

Maybe I'm misunderstanding but it is literally this method that sets the websocket to undefined, if it's already been called it should do nothing. There is no way to handle this error gracefully from the outside and the cause of it is in another library. How do you propose someone deals with this, as in our case it is recycling a live service whenever it happens?

@lpinca
Copy link
Member

lpinca commented Aug 28, 2020

There is no way to handle this error gracefully from the outside and the cause of it is in another library.

That error must not be handled gracefully. It should crash the process. We can put an assert(this[kWebSocket]); like it's done here https://github.com/nodejs/node/blob/v14.9.0/lib/_http_client.js#L507 or here https://github.com/nodejs/node/blob/v14.9.0/lib/_http_client.js#L700 for example but the result would be the same.

How do you propose someone deals with this, as in our case it is recycling a live service whenever it happens?

Fix the problem that is causing it. Fix the invalid usage of ws.

@mredbishop
Copy link

So as far as I can tell the handler is attached twice, which means the "upgrade" handler has run multiple times. Have you got any suggestions on where to look to try and get this issue resolved?

image

@lpinca
Copy link
Member

lpinca commented Aug 28, 2020

Read my comments here #1787, they should help you.

@wizzfizz94
Copy link

For me the error was thrown because I failed to remove the upgrade listener from my http server after closing a noServer web socket server. Make sure you hold a reference to the upgrade callback that you can remove later when closing the connection. E.g.

   // setup
   this.wsServer = new ws.Server({
      noServer: true
   })

  // upgrade cbk init
  this.upgradeCbk = (request, socket, head) => {
    const pathname = url.parse(request.url).pathname;
    if (pathname === this.path) {
      this.wsServer.handleUpgrade(request, socket, head, ws => {
        this.wsServer.emit('connection', ws, request);
      });
    }
  }

  this.server.on('upgrade', this.upgradeCbk);
  // later on when shutting down ws server
  this.wsServer.close()
  this.server.removeListener("upgrade", this.upgradeCbk)

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

8 participants