[core] Misc modules/* cleanup #22983
Conversation
|
No bundle size changes comparing 5ec7555...a3a0b3d |
| @@ -4,46 +4,38 @@ const http = require('http'); | |||
| const path = require('path'); | |||
| const express = require('express'); | |||
| const expect = require('expect-puppeteer'); | |||
| const { addTeardown, shutdown } = require('../../../../modules/handleKillSignals'); | |||
There was a problem hiding this comment.
Are you sure about the teardown logic? How do we guarantee that the browser is correctly closed when sending an interruption signal to the node process? (that the browser doesn't stay idle as a dead leaf)
There was a problem hiding this comment.
How long does the script run for you?
There was a problem hiding this comment.
Also: you kind of can't. Orphaned processes are hard problem so you would need to show me a concrete case where this is a problem. A general solution is too much work for a little helper script.
There was a problem hiding this comment.
6,021 ms. Yeah ok, I see your point. For context, the teardown logic was previously used in Docker node.js files. We were using it for our web Next.js instances, our API express instances, and our end-to-end tests in puppeteer. We had different teardown to run in different orders, close redis connection, close postgres connection, stop http server handler, etc. I guess its overkill here.
There was a problem hiding this comment.
Node changed a lot regarding async/await logic. I'm fairly certain it takes care of possible orphan processes (and likely always did): You don't need to manually listen for sigint since any spawned processes are closed when the node process terminates.
There was a problem hiding this comment.
Agree with that. Our main use case was more for graceful shutdown (stop the http server first, then close the other connections, like the database).
modules/Node provides native methods/syntax for these patterns. Similar to Misc review for benchmarks mnajdova/material-ui#9