Skip to content

Conversation

@mcollina
Copy link
Member

As noted by @franky47 in #45, the http server close callback does not fire until all connections are closed. This works extremely well for http requests, because it allows to wait until everything is done. It does not work well for websockets, because we need to call a method in each of them close(), hence I monkeypatch the server.close method.

I think we should send a PR to fastify to add the ability to handle this specific case.

This is a draft PR because the behavior should likely be customizable, i.e. the client.close() accepts a code and a message.

Fixes #45.

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

@mcollina mcollina requested review from Eomm and delvedor January 26, 2020 09:42
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

I’m not sure if it is worth to add a new hook since this seems an edge case related to this module.
I’m wondering if we should be more aggressive when closing in fastify itself by using something like https://www.npmjs.com/package/stoppable.

@mcollina
Copy link
Member Author

stoppable tracks all open sockets in a map. From my tests this lead to a significant overhead in term of throughput.

Note that we are already supporting that use case with the use of timeouts, so we don’t need stoppable for this to work.

We can land this and possibly add another hook or add prependHook to add an hook at the first position.

@Eomm
Copy link
Member

Eomm commented Feb 26, 2020

Could we improve this feature request fastify/fastify#1795 with a TODO list?
If we manage the socket internally a preClose should not be necessary.

Anyway, I think a preClose hook cloud be useful for users to flush some data that are in memory and not persisted yet.

@mcollina
Copy link
Member Author

@Eomm done, feel free to edit.

@delvedor @Eomm what do you think? Should we land this for v2?

@airhorns
Copy link
Member

airhorns commented Jul 7, 2020

Have you folks further developed your thinking around this? It would be super awesome to have this just work by default -- happy to contribute if I can!

@mcollina
Copy link
Member Author

mcollina commented Jul 7, 2020

I think we can land this.

@delvedor @Eomm what you think?

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Sorry for the delay

Copy link
Member

@delvedor delvedor 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 marked this pull request as ready for review July 7, 2020 16:01
@mcollina mcollina merged commit 2a746d6 into master Jul 10, 2020
@airhorns airhorns deleted the gracefulclose branch February 23, 2021 19:29
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.

Graceful exit - onClose hook is never called when connections are active

5 participants