adonisjs
change: sigkill --> sigterm to allow graceful shutdowns
🔗 Linked issue
I've started a proposal on discord: https://discord.com/channels/423256550564691970/1216131764410650674/1216131764410650674
❓ Type of change
- [ ] 🐞 Bug fix (a non-breaking change that fixes an issue)
- [X] 👌 Enhancement (improving an existing functionality like performance)
- [ ] ✨ New feature (a non-breaking change that adds functionality)
- [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
📚 Description
This changes all instances of SIGKILL to SIGTERM when killing processes. This allows the child processes to gracefully shutdown. In adonis, this means that the ServiceProvider's have their shutdown handlers invoked. With SIGKILL, I've noticed that the ServiceProvider.shutdown handler is never invoked.
To give context as to why this is an issue for me, my application needs the ServiceProvider.shutdown handler invoked in order to be successfully started back up. I have puppeteer running from within my adonis app, which spawns a child process for chromium. Puppeteer/chromium has logic in place to ensure that only one process is ever running (they use tmp dir for this). I need to kill this process before my app can restart successfully. I currently handle this via a ServiceProvider.shutdown hook. With SIGTERM, this handler is invoked, with SIGKILL, it is not.
Is there a specific reason I may be overlooking that SIGKILL was explicitly chosen? If so, please let me know and I'll close this and add in some "hacks" to my application boot to do this cleanup.
I wasn't sure how to test this. I didn't notice any existing tests around this functionality. I would be happy to add some, but I need some guidance!
What I did test was changing SIGKILL to SIGTERM on the v5.9.5 tag here: https://github.com/adonisjs/assembler/blob/9cfd8180f75948ded60171c446638b5fb6b3b23f/src/HttpServer/index.ts#L83
My app successfully reboots in node ace serve --watch with this change.
So I did not test these proposed changes on develop, as I don't have an app running adonis v6/assemblerv7!
I would really like this fixed in adonis v5 / assembler v5, but I wasn't sure of the current branching strategy to support that.
📝 Checklist
- [X] I have linked an issue or discussion.
- [ ] I have updated the documentation accordingly.
A couple of things before we move forward
- I went with
SIGKILLbecause I expect my file watcher restarts to be quick and not stuck on the graceful shutdown process, which might take a few seconds. - I would love to know how other file watchers (like nodemon) handles this. Is there any standard they follow? Would love to read some material on the same.
CC @targos @RomainLanz @Julien-R44 Would love to get your insights on the same as-well.
I went with SIGKILL because I expect my file watcher restarts to be quick and not stuck on the graceful shutdown process, which might take a few seconds.
i am a little afraid of that too. we've already had a few problems with the app startup time (and in truth, it can still be a problem, I have some apps where i have to wait a good 1 second to be full reloaded). if on top of that, we have to add 500/600ms of shutdown, then DX won't be great
but, for what it's worth, i've tested it on adonis-packages, and the shutdown of all providers only takes 10ms. the most time-consuming part has always been importing packages and parsing files. so shutdown won't be affected by this
but, for what it's worth, i've tested it on adonis-packages, and the shutdown of all providers only takes 10ms. the most time-consuming part has always been importing packages and parsing files. so shutdown won't be affected by this
Do we use any databases in the adonisjs-packages repo? Because, that is where the things can slow down a bit.
Also, many folks reported issues with Japa saying "Tests are over, but app is not shutting down". This is because, they had some open connections during the tests that they forgot to close.
So I suspect the issue will repeat here. Someone opens a connection and forgot to write the code to clean it up and now their dev-server reloads are not working and they report an issue for it.
But again, I am open for the change, if there is a standard followed by other tools.
I haven't used nodemon, however they have a CLI flag for switching the signal sent: https://github.com/remy/nodemon?tab=readme-ov-file#gracefully-reloading-down-your-script
nodemon --signal SIGHUP server.js for example.
Perhaps introducing a CLI switch would be best, as that lets us remain defaulted to SIGKILL for performance, but allow users to switch to the likes of a SIGTERM. Thoughts?
Seems like nodemon sends SIGUSR2 by default https://github.com/remy/nodemon?tab=readme-ov-file#controlling-shutdown-of-your-script.
I will do a bit of reading and then get back on this thread
I am not confident enough about Linux signals to comment on that. From an API perspective, having a flag or an environment variable allowing the final user to choose what signal he prefers seems to be a nice workaround for the issue.
