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

fix (Connection): Add error handler to websocket instance #393

Merged
merged 7 commits into from
Sep 2, 2022

Conversation

jamesopti
Copy link
Collaborator

@jamesopti jamesopti commented Aug 29, 2022

Overview

Fixes #392

Adds a listener for the error event from the underlying ws instance.

This is necessary to handle any errors emitted by an individual websocket object are caught and don't cause the server to crash.

Context

websockets/ws#1825 (comment)

websockets/ws#1777 (comment)

@svenadlung svenadlung requested review from janthurau and removed request for kriskbx and hanspagel August 30, 2022 06:45
@janthurau
Copy link
Collaborator

@jamesopti thanks a lot for your bug report and even more thanks for fixing it yourself!! 🙏

@janthurau
Copy link
Collaborator

@jamesopti Do we need to keep the updated package-json.lock? Looks like it was just generated by a new npm version probably?

tests/server/websocketError.ts Outdated Show resolved Hide resolved
tests/server/websocketError.ts Outdated Show resolved Hide resolved
@jamesopti
Copy link
Collaborator Author

@jamesopti Do we need to keep the updated package-json.lock? Looks like it was just generated by a new npm version probably?

Good catch, reverted!

How does this repo manage NPM versions? I'm using NVM and currently have 16.3 (.nvm/versions/node/v16.13.0/bin/node)

@jamesopti
Copy link
Collaborator Author

@janthurau Any ideas about the test failures here? https://github.com/ueberdosis/hocuspocus/runs/8094400524?check_suite_focus=true#step:7:555

Googling suggests this error was introduced in Node 18.6 but I think this repo runs Node 16 🤔

@janthurau
Copy link
Collaborator

janthurau commented Aug 30, 2022

@jamesopti thx! Seems like Github changed something, the tests that passed yesterday suddenly started failing as well (https://github.com/ueberdosis/hocuspocus/actions/runs/2944477556). Let's hope this resolves itself (on github side maybe), if not I'll have a look on Thursday/Friday. Weird!

@jamesopti
Copy link
Collaborator Author

jamesopti commented Aug 30, 2022

@jamesopti thx! Seems like Github changed something, the tests that passed yesterday suddenly started failing as well (https://github.com/ueberdosis/hocuspocus/actions/runs/2944477556). Let's hope this resolves itself (on github side maybe), if not I'll have a look on Thursday/Friday. Weird!

Hmmm looks like the Node version changes since yesterday from 16.16 to 16.17 (see screenshots from successful vs unsuccessful builds of this PR).

I suspect we'll need to pin to node 16.16 or fix the errors. I can reproduce locally now with v16.17

Screen Shot 2022-08-30 at 7 48 35 AM

Screen Shot 2022-08-30 at 7 45 44 AM

Screen Shot 2022-08-30 at 7 46 03 AM

@svenadlung
Copy link
Contributor

Wow, yes. We fought with this several hours on local machines already.

See
TypeStrong/ts-node#1865 and
TypeStrong/ts-node#1839

Updating ts-node leads to another error.
See https://github.com/ueberdosis/hocuspocus/runs/8095091264?check_suite_focus=true

Most people changed from ts-node to esbuild-node-loader which seems to be deprecated. Using tsm instead results in problems with chalk in particular (lukeed/tsm#31).

Not really happy about it but pinning node version seems to be the only solution right now?

@janthurau
Copy link
Collaborator

I've updated the github actions to stick with 16.16 for now and rebased this branch with main. Let's pray for the build to succeed :)

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

Successfully merging this pull request may close these issues.

Server can crash with WS_ERR_INVALID_OPCODE
3 participants