Closed airhorns closed 2 years ago
@mcollina I think technically speaking this is semver major in that errors will be handled a little bit differently, but pragmatically speaking I am not sure anyone will really notice as it is very subtle. Do you have a preference asto what the next version should be?
Let's bump the major, it's not as problematic as bumping Fastify.
fastify-websocket
has rich support for running requests through the router and hooks before actually upgrading them to full duplex websocket connections outside the normal HTTP lifecycle. Hooks are useful for auth, sessions, tracing, etc etc, and so we take care to make the request look as normal as possible until wereply.hijack()
and take over.Before this change, the
onError
hook thatfastify-websocket
was registering was very eagerly handling errors encountered during the hook chain. TheonError
hook would run before any error handlers added by.setErrorHandler
, and it would by-default upgrade the request to dispatch a close code. This changed with the upgrade to fastify v4 -- before, any error in a hook would run the.setErrorHandler
errors first, which would probably reply.send, and the request would never get hijacked.I think the prior behaviour is better for two reasons. First, hook code that is doing auth or sessions or tracing or whatever is going to tend to presume it can
reply.send
to report an error. I think we should let it instead of having to make every error handler in your app be websocket aware. And second, it is likely to reply.send with a useful response because it is the endorsed way of reporting errors, whereas fastify-websocket's default error handling doesn't really know much about what to do in the face of errors and handles them really generically.The good news is restoring the fastify v3 behaviour for this module is easy -- we just delete the
onError
event handler from this library. I think it never really made sense after we switched to dispatching requests through the fastify before upgrading them -- in that phase, the request isn't yet special. The tests pass fine without it on fastify v4. We let fastify's normal error event handling do it's thing before the request gets upgraded. The behavior after upgrade is unchanged, which is to run the websocket specific error handler passed in as an option to this plugin.Also of note that without making this change, if you
reply.send
in asetErrorHandler
error handler, you end up double sending because the.onError
hook hijacks the reply, and then yoursetErrorHandler
code runs and encountersFST_ERR_REP_ALREADY_SENT
. Here's the log of me running the new test this PR adds on the old code: