Better SIGINT handling in chrome-launcher #2959
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
|
Thanks for the PR @mikecardwell! This lines up well with the approach @paulirish takes in #2616 of moving toward a manager for multiple instances. |
| process.exit(_SIGINT_EXIT_CODE); | ||
| }); | ||
| if (opts.handleSIGINT && instances.size === 0) { | ||
| process.on(_SIGINT, sigintListener); |
There was a problem hiding this comment.
@paulirish @JoelEinbinder this gonna yield "max listeners" error in case of 11 simultaneously launched browsers.
There was a problem hiding this comment.
instances is a global variable, so it only adds one listener per project.
There was a problem hiding this comment.
ah i see. So the "handleSIGINT" option is not per-browser but per-process.
This is a fix for #2797
Currently we add a SIGINT handler for each launched Chrome instance to kill that instance on exit. My change makes it so that we track the list of running Chrome instances. When that list goes from 0 to 1, a SIGINT handler is added which kills all tracked Chrome instances. When that list goes from 1 to 0, the handler is removed.
So there is only ever a maximum of one SIGINT listener, which prevents the EventEmitter memory leak warnings.
I believe that my fix is better than the other one provided in the issue as it allows for you to have more than 10 simultaneously running Chrome instances without an EventEmitter memory leak warning.