Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure shutdown signals aren't trapped forever #313

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Tyler-Murphy
Copy link

Right now, it seems like this library is preventing the SIGINT (ctrl-C) signal from shutting down my project.

I made these changes using github's edit button, so I haven't tested/linted anything, and there could be mistakes. I have confirmed that making these changes causes SIGINT to shut down my project like I expect it to.

See this comment for an explanation: berstend/puppeteer-extra#467 (comment). Here are the comment's contents at the time of writing, in case the link stops working:

I've run into problems like this before, where a process.on listener traps SIGINT every time it's sent.

Reproduce the problem with this script. Run it and try hitting ctrl-C repeatedly. It'll be trapped every time and the process won't exit.

process.on(`SIGINT`, () => console.log(`caught SIGINT`))
setInterval(() => {}, 1e3) // do work to prevent immediate exit

I've fixed this a couple ways in the past.

  1. Use process.once, and after cleanup work is complete, print a message saying something like "send SIGINT again to exit".
  2. Re-emit the kill signal after cleanup work is complete.

Option 2 seems like a better fit in this case, and it works better in conjunction with other, unrelated cleanup scripts with their own SIGINT listeners. Here's what it looks like:

process.once(`SIGINT`, () => {
	console.log(`caught SIGINT`)
	process.kill(process.pid, `SIGINT`) // send `SIGINT` back to self
})
setInterval(() => {}, 1e3) // do work

I tried replacing the puppeteer-extra-plugin process.on listeners in the index file with process.once listeners as described, and it seems to fix the problem for me.

For example:

if (this.onClose) {
                if (opts.options.handleSIGINT !== false) {
                    process.once('SIGINT', () => {
                        this.onClose()
                        process.kill(process.pid, `SIGINT`)
                    });
                }
                ...

Note: I haven't tested this against the other test cases in this thread, but it solved my problem. Maybe others can see if it works for them, too.

Right now, it seems like this library is preventing the `SIGINT` (ctrl-C) signal from shutting down my project.

I made these changes using github's edit button, so I haven't tested/linted anything, and there could be mistakes.

See this comment for an explanation: berstend/puppeteer-extra#467 (comment). Here are the comment's contents at the time of writing, in case the link stops working:

I've run into problems like this before, where a `process.on` listener traps `SIGINT` every time it's sent.

Reproduce the problem with this script. Run it and try hitting `ctrl-C` repeatedly. It'll be trapped every time and the process won't exit.

```node
process.on(`SIGINT`, () => console.log(`caught SIGINT`))
setInterval(() => {}, 1e3) // do work to prevent immediate exit
```

I've fixed this a couple ways in the past.
1. Use `process.once`, and after cleanup work is complete, print a message saying something like "send SIGINT again to exit".
2. Re-emit the kill signal after cleanup work is complete.

Option 2 seems like a better fit in this case, and it works better in conjunction with other, unrelated cleanup scripts with their own `SIGINT` listeners. Here's what it looks like:

```node
process.once(`SIGINT`, () => {
	console.log(`caught SIGINT`)
	process.kill(process.pid, `SIGINT`) // send `SIGINT` back to self
})
setInterval(() => {}, 1e3) // do work
```

I tried replacing [the `puppeteer-extra-plugin` `process.on` listeners in the `index` file](https://github.com/berstend/puppeteer-extra/blob/0049d6010311505f27e7f3be804bb198e2c09aa2/packages/puppeteer-extra-plugin/src/index.ts#L506-L519) with `process.once` listeners as described, and it seems to fix the problem for me.

For example:

```node
if (this.onClose) {
                if (opts.options.handleSIGINT !== false) {
                    process.once('SIGINT', () => {
                        this.onClose()
                        process.kill(process.pid, `SIGINT`)
                    });
                }
                ...
```

**Note:** I haven't tested this against the other test cases in this thread, but it solved my problem. Maybe others can see if it works for them, too.
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.

1 participant