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

fix: capture websocket server errors #131

Merged
merged 2 commits into from
Jan 27, 2021
Merged

fix: capture websocket server errors #131

merged 2 commits into from
Jan 27, 2021

Conversation

lynxtaa
Copy link
Contributor

@lynxtaa lynxtaa commented Jan 27, 2021 •
edited
Loading

Fixes #130

Checklist

jsumners
jsumners reviewed Jan 27, 2021
View reviewed changes
index.js Show resolved Hide resolved
mcollina
mcollina requested changes Jan 27, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a unit test for this?

@lynxtaa
Copy link
Contributor Author

lynxtaa commented Jan 27, 2021 •
edited
Loading

@mcollina , Done

mcollina
mcollina reviewed Jan 27, 2021
View reviewed changes
test/websocket.js

const appPort = app.server.address().port

await t.rejects(app2.listen(appPort), /EADDRINUSE/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little lost on this test. I don't think it's the websocket server that errors, but fastify itself. Can you check/confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can run this test on master: it won't pass because Websocket server will cause unhandledRejection and crash test suite

mcollina
mcollina approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit ec58a4e into fastify:master Jan 27, 2021
@lynxtaa lynxtaa deleted the capture-ws-errors branch January 27, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsumners jsumners jsumners left review comments

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Websocket server errors are not captured
3 participants
@lynxtaa @mcollina @jsumners