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
fix: change kill to signal the whole process group to terminate #6859
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Ah, the CI is kind enough to provide some answers regarding the windows platform. |
@TimvdLippe PTAL since this refers to your earlier patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's unfortunate, sorry for causing unexpected regressions. At this point, I think we would benefit from having several tests that we can run to ensure that numerous process configurations and launches are behaving as expected.
Sadly I don't have the cycles myself to work on that, but @petuomin would you in a position to do so?
Ideally, we have a set of tests that can verify that launching Puppeteer as the main process, in bash scripts and on multiple platforms and that all processes are cleaned up appropriately. I am not aware of prior art in this area and I especially don't really know how you would be able to obtain process information on platforms such as Windows. Let me know if you are up for the challenge and I would happily assist wherever possible!
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
No worries, we were able to figure out a workaround for us for now. I'll try to find some time to work on this. It might take a while though as I, too, will have to learn some windows. |
838440c
to
0ff2144
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
03a4ca1
to
d4b17bd
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me! As it's basically restoring previous (also untested) behaviour, I would be happy to land this for now and figure out a better way to test this in a follow up change. Let me know what you think @TimvdLippe
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@petuomin It looks like the @googlebot couldn't verify that you signed the cla yet. Could maybe check if you did and post another |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@petuomin Also, it looks like your change doesn't quite work on windows yet - could you also look into that? |
@jschfflr I am happy with relanding, but I don't think the current fix works quite yet. Unfortunately I don't have the resources to check what is going on, but I have no objections to this PR 😄 |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@petuomin I changed the implementation to ignore |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
14bac19
to
fa8aacc
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I consent! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@petuomin Could you make sure to sign the CLA with the email address you used to create the commit? |
fa8aacc
to
a9395f7
Compare
a9395f7
to
8b09fcc
Compare
8b09fcc
to
7b5cccc
Compare
In the PR that improves the
ctrl+c
, the signal handling may have been accidentally changed to kill just the process instead of the process group.We are using puppeteer to launch a bash script that starts xvfb and then chrome. Since the bash script receives the SIGKILL, it has no change to do cleanup, leaving a dangling xvfb. Signaling the process group fixes the issue.
Note: I'm not familiar with Windows. The code in this PR is from BrowserRunner.ts before PR6011 was applied. It seems like it should be right based on this: https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/taskkill
Note2: The nodejs documentation states: "Sending signal 0 can be used as a platform independent way to test for the existence of a process." However the documentation does not state whether it will throw an error with code
ESRCH
on Windows.