fastify / fastify-websocket

basic websocket support for fastify
MIT License
394 stars 72 forks source link

Allow specifying alternative preClose / modifying close flow #260

Closed HDegroote closed 1 year ago

HDegroote commented 1 year ago

Prerequisites

🚀 Feature Proposal

The current exit flow is:

I'd propose to change it to something like:

Motivation

I have a usecase where I would like the websocket server to shutdown as follows:

This is not hard to implement in my own preClose handler. I managed to get around not being able to provide my own preClose handler by registering it before registering fastify-websocketServer so it runs earlier, but that's quite hacky, and my current solution causes the close handler to error out on server.close(done), because the ws server is already closed.

For context, this is the gist of my preClose hook.

app.addHook('preClose', async function () {
    const closeProm = new Promise(resolve => wsServer.close(resolve))
    closeProm.catch(() => {})

    for (const socket of wsServer.clients) {
      socket.send(`Server closing. Socket will shut down in ${sShutdownMargin}s`)
    }

    await Promise.race([
      new Promise(resolve => setTimeout(resolve, sShutdownMargin * 1000)),
      closeProm // If all connections close before the timeout
    ])

    const goingAwayCode = 1001
    for (const client of wsServer.clients) {
      client.close(goingAwayCode, 'Server is going offline')
    }

   await closeProm

If you're open to this feature, I'm fine with PR'ing whichever variation you deem fit. Or if you want to go a bit further, fastify-websocket could also support this kind of shutdown natively, by adding something like sShutdownMargin and onShutdownMessage options

Example

fastify.register(require('@fastify/websocket'), {
  options: {
    preClose: myOwnPreClose    
  }

or

fastify.register(require('@fastify/websocket'), {
  options: {
    sShutdownMargin: 10,
    onShutdownMessage: 'Shutting down in 10 seconds'    
  }
mcollina commented 1 year ago

Would you like to send a Pull Request to address this issue? Remember to add unit tests. Providing a user-defined preClose option is the way to go.

HDegroote commented 1 year ago

Cool, thanks! Yes, I'll create a PR.

Are you fine with the proposed update to the default closing flow, where it no longer has a close hook but instead closes the ws server during the preClose ?