Skip to content

Conversation

airhorns
Copy link
Member

This change fixes a bug in fastify-websocket where the reply object wouldn't actually send anything if it was asked to -- the only think you could do with it is hijack it. Before this change, if you tried to do reply.send or reply.code or what have you, those bytes would never make it out the client. I tested with browsers and with wsocat. The effect of this is that if someone is using an error handler to send errors as HTTP responses, or a validation handler that authenticates or something like that, those responses never make it to the client, but fastify considers the reply sent and is finished with it, leaving hanging sockets and broken expectations.

I think this is an accidental change introduced in #95 when we switched this library to start using fastify's routing and creating this reply object where we never quite set it up right. Now, we do a response.assignSocket with the socket given to us by the websocket server so that the response object can write to it.

Checklist

This change fixes a bug in fastify-websocket where the `reply` object wouldn't actually send anything if it was asked to -- the only think you could do with it is hijack it. Before this change, if you tried to do reply.send or reply.code or what have you, those bytes would never make it out the client. I tested with browsers and with wsocat. The effect of this is that if someone is using an error handler to send errors as HTTP responses, or a validation handler that authenticates or something like that, those responses never make it to the client, but fastify considers the reply sent and is finished with it, leaving hanging sockets and broken expectations.

I think this is an accidental change introduced in #95 when we switched this library to start using fastify's routing and creating this reply object where we never quite set it up right. Now, we do a `response.assignSocket` with the socket given to us by the websocket server so that the response object can write to it.
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 a0ec207 into master Feb 27, 2022
@airhorns airhorns deleted the early-send branch February 27, 2022 15:15
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.

3 participants