Ensure that setErrorHandler functions run for errors thrown before th…

…e websocket is upgraded

`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 we `reply.hijack()` and take over.

Before this change, the `onError` hook that `fastify-websocket` was registering was very eagerly handling errors encountered during the hook chain. The `onError` 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. 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 a `setErrorHandler` error handler, you end up double sending because the `.onError` hook hijacks the reply, and then your `setErrorHandler` code runs and encounters `FST_ERR_REP_ALREADY_SENT`. Here's the log of me running the new test this PR adds on the old code:

```
test/hooks.js 1> [1659567917980] INFO (59599 on claw.localdomain): Server listening at http://127.0.0.1:50104
test/hooks.js 1> [1659567917981] INFO (59599 on claw.localdomain): Server listening at http://[::1]:50104
test/hooks.js 1> [1659567917991] INFO (59599 on claw.localdomain): incoming request
test/hooks.js 1>     reqId: "req-1"
test/hooks.js 1>     req: {
test/hooks.js 1>       "method": "GET",
test/hooks.js 1>       "url": "/echo",
test/hooks.js 1>       "hostname": "localhost:50104",
test/hooks.js 1>       "remoteAddress": "127.0.0.1",
test/hooks.js 1>       "remotePort": 50105
test/hooks.js 1>     }
test/hooks.js 1> [1659567917993] ERROR (59599 on claw.localdomain): Fail
test/hooks.js 1>     reqId: "req-1"
test/hooks.js 1>     err: {
test/hooks.js 1>       "type": "Error",
test/hooks.js 1>       "message": "Fail",
test/hooks.js 1>       "stack":
test/hooks.js 1>           Error: Fail
test/hooks.js 1>               at Object.<anonymous> (/Users/airhorns/Code/fastify-websocket/test/hooks.js:94:13)
test/hooks.js 1>               at processTicksAndRejections (node:internal/process/task_queues:96:5)
test/hooks.js 1>     }
test/hooks.js 1> [1659567917996] WARN (59599 on claw.localdomain): Reply already sent
test/hooks.js 1>     reqId: "req-1"
test/hooks.js 1>     err: {
test/hooks.js 1>       "type": "FastifyError",
test/hooks.js 1>       "message": "Reply was already sent.",
test/hooks.js 1>       "stack":
test/hooks.js 1>           FastifyError: Reply was already sent.
test/hooks.js 1>               at Reply.send (/Users/airhorns/Code/fastify-websocket/node_modules/fastify/lib/reply.js:118:26)
test/hooks.js 1>               at Object.<anonymous> (/Users/airhorns/Code/fastify-websocket/test/hooks.js:99:29)
test/hooks.js 1>               at handleError (/Users/airhorns/Code/fastify-websocket/node_modules/fastify/lib/error-handler.js:58:18)
test/hooks.js 1>               at /Users/airhorns/Code/fastify-websocket/node_modules/fastify/lib/reply.js:670:13
test/hooks.js 1>               at done (/Users/airhorns/Code/fastify-websocket/node_modules/fastify/lib/hooks.js:213:7)
test/hooks.js 1>               at Object.<anonymous> (/Users/airhorns/Code/fastify-websocket/index.js:93:5)
test/hooks.js 1>               at next (/Users/airhorns/Code/fastify-websocket/node_modules/fastify/lib/hooks.js:219:30)
test/hooks.js 1>               at onSendHookRunner (/Users/airhorns/Code/fastify-websocket/node_modules/fastify/lib/hooks.js:241:3)
test/hooks.js 1>               at onErrorHook (/Users/airhorns/Code/fastify-websocket/node_modules/fastify/lib/reply.js:665:5)
test/hooks.js 1>               at Reply.send (/Users/airhorns/Code/fastify-websocket/node_modules/fastify/lib/reply.js:124:5)
test/hooks.js 1>       "name": "FastifyError",
test/hooks.js 1>       "code": "FST_ERR_REP_ALREADY_SENT",
test/hooks.js 1>       "statusCode": 500
test/hooks.js 1>     }
```

README.md

@@ -192,11 +192,11 @@ fastify.listen(3000, err => {
 
 ### Custom error handler:
 
-You can optionally provide a custom errorHandler that will be used to handle any cleaning up:
+You can optionally provide a custom `errorHandler` that will be used to handle any cleaning up of established websocket connections. The `errorHandler` will be called if any errors are thrown by your websocket route handler after the connection has been established. Note that neither Fastify's `onError` hook or functions registered with `fastify.setErrorHandler` will be called for errors thrown during a websocket request handler.
 
-```js
-'use strict'
+Neither the `errorHandler` passed to this plugin or fastify's `onError` hook will be called for errors encountered during message processing for your connection. If you want to handle unexpected errors within your `message` event handlers, you'll need to use your own `try { } catch {}` statements and decide what to send back over the websocket.
 
+```js
 const fastify = require('fastify')()
 
 fastify.register(require('@fastify/websocket'), {
@@ -231,6 +231,7 @@ fastify.listen(3000, err => {
 })
 ```
 
+Note: Fastify's `onError` and error handlers registered by `setErrorHandler` will still be called for errors encountered *before* the websocket connection is established. This means errors thrown by `onRequest` hooks, `preValidation` handlers, and hooks registered by plugins will use the normal error handling mechanisms in Fastify. Once the websocket is established and your websocket route handler is called, `fastify-websocket`'s `errorHandler` takes over.
 ## Options
 
 `@fastify/websocket` accept these options for [`ws`](https://github.com/websockets/ws/blob/master/doc/ws.md#new-websocketserveroptions-callback) :

index.js

@@ -80,19 +80,6 @@ function fastifyWebsocket (fastify, opts, next) {
     done()
   })
 
-  fastify.addHook('onError', (request, reply, error, done) => {
-    /* istanbul ignore next */
-    if (request.raw[kWs]) {
-      // Hijack reply to prevent fastify from sending the error after onError hooks are done running
-      reply.hijack()
-      handleUpgrade(request.raw, connection => {
-        // Handle the error
-        errorHandler.call(this, error, connection, request, reply)
-      })
-    }
-    done()
-  })
-
   fastify.addHook('onResponse', (request, reply, done) => {
     if (request.ws) {
       request.raw[kWs].destroy()

test/hooks.js

@@ -93,16 +93,14 @@ test('Should run onError hook before handler is executed (error thrown in onRequ
     fastify.addHook('onError', async (request, reply) => t.ok('called', 'onError'))
 
     fastify.get('/echo', { websocket: true }, (conn, request) => {
-      t.teardown(conn.destroy.bind(conn))
+      t.fail()
     })
   })
 
   fastify.listen({ port: 0 }, function (err) {
     t.error(err)
     const ws = new WebSocket('ws://localhost:' + (fastify.server.address()).port + '/echo')
-    const client = WebSocket.createWebSocketStream(ws, { encoding: 'utf8' })
-    t.teardown(client.destroy.bind(client))
-    ws.on('close', code => t.equal(code, 1006))
+    ws.on('unexpected-response', (_request, response) => t.equal(response.statusCode, 500))
   })
 })
 
@@ -130,9 +128,7 @@ test('Should run onError hook before handler is executed (error thrown in preVal
   fastify.listen({ port: 0 }, function (err) {
     t.error(err)
     const ws = new WebSocket('ws://localhost:' + (fastify.server.address()).port + '/echo')
-    const client = WebSocket.createWebSocketStream(ws, { encoding: 'utf8' })
-    t.teardown(client.destroy.bind(client))
-    ws.on('close', code => t.equal(code, 1006))
+    ws.on('unexpected-response', (_request, response) => t.equal(response.statusCode, 500))
   })
 })
 
@@ -152,7 +148,7 @@ test('onError hooks can send a reply and prevent hijacking', t => {
 
     fastify.addHook('onError', async (request, reply) => {
       t.ok('called', 'onError')
-      await reply.code(404).send('there was an error')
+      await reply.code(501).send('there was an error')
     })
 
     fastify.get('/echo', { websocket: true }, (conn, request) => {
@@ -163,9 +159,39 @@ test('onError hooks can send a reply and prevent hijacking', t => {
   fastify.listen({ port: 0 }, function (err) {
     t.error(err)
     const ws = new WebSocket('ws://localhost:' + (fastify.server.address()).port + '/echo')
-    const client = WebSocket.createWebSocketStream(ws, { encoding: 'utf8' })
-    t.teardown(client.destroy.bind(client))
-    ws.on('close', code => t.equal(code, 1006))
+    ws.on('unexpected-response', (_request, response) => t.equal(response.statusCode, 501))
+  })
+})
+
+test('setErrorHandler functions can send a reply and prevent hijacking', t => {
+  t.plan(4)
+  const fastify = Fastify()
+
+  t.teardown(() => fastify.close())
+
+  fastify.register(fastifyWebsocket)
+
+  fastify.register(async function (fastify) {
+    fastify.addHook('preValidation', async (request, reply) => {
+      await Promise.resolve()
+      throw new Error('Fail')
+    })
+
+    fastify.setErrorHandler(async (error, request, reply) => {
+      t.ok('called', 'onError')
+      t.ok(error)
+      await reply.code(501).send('there was an error')
+    })
+
+    fastify.get('/echo', { websocket: true }, (conn, request) => {
+      t.fail()
+    })
+  })
+
+  fastify.listen({ port: 0 }, function (err) {
+    t.error(err)
+    const ws = new WebSocket('ws://localhost:' + (fastify.server.address()).port + '/echo')
+    ws.on('unexpected-response', (_request, response) => t.equal(response.statusCode, 501))
   })
 })