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

fix: change kill to signal the whole process group to terminate #6859

Merged
merged 2 commits into from Feb 17, 2022

Conversation

petuomin
Copy link
Contributor

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.

@google-cla
Copy link

google-cla bot commented Feb 10, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Feb 10, 2021
@petuomin
Copy link
Contributor Author

Ah, the CI is kind enough to provide some answers regarding the windows platform.

@mathiasbynens
Copy link
Member

@TimvdLippe PTAL since this refers to your earlier patch

Copy link
Contributor

@TimvdLippe TimvdLippe left a 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!

@petuomin
Copy link
Contributor Author

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Feb 24, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@petuomin
Copy link
Contributor Author

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.

@google-cla
Copy link

google-cla bot commented Feb 24, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Sep 11, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

@jschfflr jschfflr left a 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

@jschfflr
Copy link
Contributor

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Sep 13, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jschfflr
Copy link
Contributor

@petuomin It looks like the @googlebot couldn't verify that you signed the cla yet. Could maybe check if you did and post another @googlebot I signed it!. Thanks!

@google-cla
Copy link

google-cla bot commented Sep 13, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jschfflr
Copy link
Contributor

@petuomin Also, it looks like your change doesn't quite work on windows yet - could you also look into that?

@TimvdLippe
Copy link
Contributor

@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 😄

@google-cla
Copy link

google-cla bot commented Sep 21, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jschfflr
Copy link
Contributor

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Sep 21, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jschfflr
Copy link
Contributor

@petuomin I changed the implementation to ignore taskkill failures as it looks like all processes will be successfully cleaned on windows anyways.
Please make sure to get the CLA signed and I'm happy to get this landed!

@google-cla
Copy link

google-cla bot commented Sep 21, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Sep 21, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Oct 4, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jschfflr
Copy link
Contributor

jschfflr commented Oct 4, 2021

@googlebot I consent!

@google-cla
Copy link

google-cla bot commented Oct 4, 2021

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jschfflr
Copy link
Contributor

jschfflr commented Oct 4, 2021

@petuomin Could you make sure to sign the CLA with the email address you used to create the commit?
It looks like the google bot didn't catch that one yet :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants