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 websocket auto-response message races #1309

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

jqmmes
Copy link
Contributor

@jqmmes jqmmes commented Oct 16, 2023

This PR fixes racing issues between regular websocket messages and auto-responses.

The initial version of this PR held a jsg::Ref<> for a long time, which could cause occasional segfaults. This new version fixes that segfault.

@jqmmes jqmmes force-pushed the joaquim/ws-autoresponse-segfault-fix branch 4 times, most recently from 65863fb to 8a5c005 Compare October 16, 2023 18:12
@jqmmes jqmmes force-pushed the joaquim/ws-autoresponse-segfault-fix branch 2 times, most recently from 813a381 to 41f4b54 Compare October 16, 2023 19:34
Copy link
Contributor

@bcaimano bcaimano left a comment

Choose a reason for hiding this comment

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

LGTM as long as it passes internal CI! Thanks for your work.

@jqmmes jqmmes force-pushed the joaquim/ws-autoresponse-segfault-fix branch from 41f4b54 to efc8ac0 Compare October 17, 2023 09:42
This PR fixes racing issues between regular websocket messages and auto-responses.
@jqmmes jqmmes force-pushed the joaquim/ws-autoresponse-segfault-fix branch from efc8ac0 to c08ad41 Compare October 17, 2023 09:42
@jqmmes
Copy link
Contributor Author

jqmmes commented Oct 17, 2023

Thank you, @bcaimano and @MellowYarker for the very fast review!

Copy link
Contributor

@MellowYarker MellowYarker left a comment

Choose a reason for hiding this comment

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

I left some comments but they're mostly for cleanup/maintainability. PR can be merged if you've got other stuff to focus on, thanks for fixing this!

@jqmmes jqmmes force-pushed the joaquim/ws-autoresponse-segfault-fix branch from c08ad41 to 6053b8e Compare October 17, 2023 14:29
@MellowYarker
Copy link
Contributor

Thanks for the cleanup!

Co-authored-by: James M Snell <jsnell@cloudflare.com>
@jqmmes jqmmes merged commit 413fbda into main Oct 23, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants