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.

Sign up for GitHub

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

Jump to bottom

if this[kWebSocket] = undefined , then must check it exists else where #1606

Closed
wants to merge 1 commit into from
Closed

if this[kWebSocket] = undefined , then must check it exists else where #1606

wants to merge 1 commit into from

Commits on Jul 12, 2019

  1. if this[kWebSocket] = undefined , then must check it exists else where 

    `ws` is fantastic, and devs/users of my Open Source library have been using my library with `ws` in it for years in production, they currently have like ~14M monthly active users! None of these devs/users are paying me commercially though, so this patch is on my own OSS time for their sake, but I've been very thankful that `ws` is also OSS like my library is, it is very much appreciated.

I'm issuing this patch because I cannot catch these errors that happen when I (1) run a websocket server on localhost:8765, (2) connect browser to websocket server (3) connect websocket server to a non-existent localhost:8080 & retry every 2 seconds (4) refresh browser & reconnect to localhost:8765 . Sometimes I may need a `dgram` socket running in the same instance, but it also crashes when I disable that (though maybe flooding the router with a bunch of UDP packets may be necessary to replicate).

```
node_modules/ws/lib/websocket.js:837
  websocket.readyState = WebSocket.CLOSING;
                       ^
TypeError: Cannot set property 'readyState' of undefined
    at Socket.socketOnClose
```

Either way, setting `this[kWebSocket] = undefined` seems dangerous (when most of the listeners start with a `this[kWebSocket]`) unless it is **impossible** for the listeners to be called again (which clearly is not the case, since I'm getting a crash where `this[kWebSocket]` is `undefined` which can only be possible if `socketOnClose` was called (and clearly `this.removeListener('close', socketOnClose);` is not working correctly).

I suspect that somehow the listeners are still active, so that should separately be fixed (but I'm not sure how), but the `if(!websocket){ return }` are safeguards. If the rest of the code works as intended, these lines will not break anything. If the code does not work as intended, these lines at least **prevent** an un-catch-able crash on production servers, which is really really important for me - so I hope this can be pulled & published as the solution, or pulled & published until another fix comes along.

Thanks again!
    @amark
    amark committed Jul 12, 2019
    Configuration menu
    Copy the full SHA
    a4f4f06 View commit details
    Browse the repository at this point in the history