[BUG] Node script isn't killable if importing chromium

[mhkeller]

Context:

System:

• OS: macOS 12.6.1
• Memory: 250.61 MB / 32.00 GB

Binaries:

• Node: 16.16.0 - /usr/local/bin/node
• npm: 8.11.0 - /usr/local/bin/npm

Languages:

• Bash: 3.2.57 - /bin/bash

npmPackages:

• playwright: ^1.28.1 => 1.28.1

Code Snippet

Reproduction repo: https://github.com/mhkeller/playwright-process-bug

Describe the bug

If you import { chromium } from 'playwright', the parent node process can no longer be exited with control-c. Steps to reproduce

1. Clone the test repo
2. Run node index.js
3. Try to kill the process in the terminal with control-c. It will not work

To fix it, comment out the chromium import line in my-module.js.

https://user-images.githubusercontent.com/498744/207184711-d8e9a9db-93b0-47b3-872a-9b33a88267d1.mov

[yury-s]

It doesn't seem to be playwright specific, the following code also fails to exit on Ctrl+C:

process.on('SIGINT', () =>  {
	process.exit(2);
})

for (let i = 0; i < 1_000_000; i += 1) {
	console.log(i);
}

It doesn't hang for some other imports though, so I wonder what is that specific to playwright package that makes node ignore sigint.

[mhkeller]

Testing both of these on node v18.12.1 and control-c does properly exit the script – although in the rest repo (the scenario using playwright) it takes about 7-9 seconds to exit. In your version with the process.on listener, it exits in about 1-3 seconds.

It would be interesting to see what about playwright is causing that to happen – especially since there is no code being executed – just the import.

[mhkeller]

This issue is informative: https://github.com/nodejs/node/issues/9050

So this isn't a bug per se but it seems that there is a process listener somewhere in the code that could be handled better.

[yury-s]

It would be interesting to see what about playwright is causing that to happen – especially since there is no code being executed – just the import.

Yeah, I am not aware of any sigint handlers that playwright would add during its initialization (we use some but all of them are related to child process launches), so my hunch is that a third-party dependency might be adding such a listener.

[yury-s]

Ok, the culprit is signal-exit package which we indirectly import via the following dependency chain:

[email protected] /home/user/playwright/packages/playwright-core/bundles/utils
└─┬ [email protected]
  └── [email protected]

that module unconditionally installs signal handlers during its initialization . This is a very interesting design choice but given that it only causes a problem when you have a toplevel tight loop fixing it will probably be a lower priority for us and I don't see an easy work around at this point. Let me bring this is up in the team meeting tomorrow.

[mhkeller]

Thanks for the quick and thorough investigation! That's interesting and I see that repo has some issues about this, too. My use case is that I use playwright in this package where the workflow is:

1. Read in a csv
2. Loop through the rows to do some cleaning
3. Render a chart with playwright

So hence why I'm using a toplevel loop. Thanks again for looking into it!

[yury-s]

We've decided in the team meeting that it would make sense for playwright to fix the lock file support and make it not depend on signal-exit code.

[mhkeller]

Very exciting – thanks for your time in working on it.

[mhkeller]

Thanks for your work on this @aslushnikov @yury-s @dgozman ! I'm looking forward to using it in the next release