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

(PUP-9092) Improve process handling on Windows (corrupted by author, see #7038) #7033

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@onyxmaster

onyxmaster commented Aug 30, 2018

A simple improvement to improve process handling rate on Windows.
With exec/onlyif/unless-heavy catalogs, this change easily makes catalog application 10 times faster.

@puppetlabs-jenkins

This comment has been minimized.

Show comment
Hide comment
@puppetlabs-jenkins

puppetlabs-jenkins Aug 30, 2018

Collaborator

Can one of the admins verify this patch?

Collaborator

puppetlabs-jenkins commented Aug 30, 2018

Can one of the admins verify this patch?

@puppetcla

This comment has been minimized.

Show comment
Hide comment
@puppetcla

puppetcla Aug 30, 2018

Waiting for CLA signature by @onyxmaster

@onyxmaster - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppet.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppet.com/community/trivial_patch_exemption.html

puppetcla commented Aug 30, 2018

Waiting for CLA signature by @onyxmaster

@onyxmaster - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppet.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppet.com/community/trivial_patch_exemption.html

@onyxmaster

This comment has been minimized.

Show comment
Hide comment
@onyxmaster

onyxmaster Aug 30, 2018

Your email sending procedure appears to be broken, I never got the email, no matter how much I clicked “resend”, or change my email address. Yes, I checked my spam folder, no, I do not have any filters.

onyxmaster commented Aug 30, 2018

Your email sending procedure appears to be broken, I never got the email, no matter how much I clicked “resend”, or change my email address. Yes, I checked my spam folder, no, I do not have any filters.

@puppetcla

This comment has been minimized.

Show comment
Hide comment
@puppetcla

puppetcla Aug 30, 2018

CLA signed by all contributors.

puppetcla commented Aug 30, 2018

CLA signed by all contributors.

@joshcooper

This comment has been minimized.

Show comment
Hide comment
@joshcooper

joshcooper Aug 30, 2018

Member

Hi @onyxmaster, thank you for your contribution! The change makes sense. I think the only downside is the ruby thread can't be interrupted for up to 15 seconds once it calls into the native method (assuming the child process is long lived). But maybe newer ruby versions handle that correctly now?

About which version to target, the 4.10.x is in security mode only and 5.3.x is in deep maintenance mode. Please target the 5.5.x base branch.

Member

joshcooper commented Aug 30, 2018

Hi @onyxmaster, thank you for your contribution! The change makes sense. I think the only downside is the ruby thread can't be interrupted for up to 15 seconds once it calls into the native method (assuming the child process is long lived). But maybe newer ruby versions handle that correctly now?

About which version to target, the 4.10.x is in security mode only and 5.3.x is in deep maintenance mode. Please target the 5.5.x base branch.

@onyxmaster

This comment has been minimized.

Show comment
Hide comment
@onyxmaster

onyxmaster Aug 30, 2018

IIRC, the 2nd argument to WaitForSingleObject is specified in milliseconds, also my manual testing of Ctrl-C-ing puppet while it executes a specially built binary that loops for 30 seconds worked instantly every time (although I did only three tests).

onyxmaster commented Aug 30, 2018

IIRC, the 2nd argument to WaitForSingleObject is specified in milliseconds, also my manual testing of Ctrl-C-ing puppet while it executes a specially built binary that loops for 30 seconds worked instantly every time (although I did only three tests).

@joshcooper

This comment has been minimized.

Show comment
Hide comment
@joshcooper

joshcooper Aug 30, 2018

Member

2nd argument to WaitForSingleObject is specified in milliseconds

Ah right, that makes sense 👍

Member

joshcooper commented Aug 30, 2018

2nd argument to WaitForSingleObject is specified in milliseconds

Ah right, that makes sense 👍

@hlindberg

This comment has been minimized.

Show comment
Hide comment
@hlindberg

hlindberg Aug 31, 2018

Contributor

Yep, second arg to WaitForSingleObject is in ms.

Contributor

hlindberg commented Aug 31, 2018

Yep, second arg to WaitForSingleObject is in ms.

@joshcooper joshcooper changed the base branch from master to 5.5.x Aug 31, 2018

@joshcooper joshcooper changed the base branch from 5.5.x to master Aug 31, 2018

@joshcooper

This comment has been minimized.

Show comment
Hide comment
@joshcooper

joshcooper Aug 31, 2018

Member

@onyxmaster could you rebase your PR on 5.5.x?

Member

joshcooper commented Aug 31, 2018

@onyxmaster could you rebase your PR on 5.5.x?

@onyxmaster onyxmaster closed this Sep 2, 2018

@puppetlabs-jenkins

This comment has been minimized.

Show comment
Hide comment
@puppetlabs-jenkins

puppetlabs-jenkins Sep 2, 2018

Collaborator

Can one of the admins verify this patch?

Collaborator

puppetlabs-jenkins commented Sep 2, 2018

Can one of the admins verify this patch?

@onyxmaster onyxmaster changed the title from (PUP-9092) Improve process handling on Windows to (PUP-9092) Improve process handling on Windows (corrupted by author, see #7038) Sep 2, 2018

@onyxmaster

This comment has been minimized.

Show comment
Hide comment
@onyxmaster

onyxmaster Sep 2, 2018

Well, seems that Github believes that force pushing wasn't a good idea, there are now no new commits between my master and origin 5.5.x, so I cant change the PR base, and it closed the PR automatically, see the new PR. Sorry for any inconvenience, I'm "not really good" with git.

onyxmaster commented Sep 2, 2018

Well, seems that Github believes that force pushing wasn't a good idea, there are now no new commits between my master and origin 5.5.x, so I cant change the PR base, and it closed the PR automatically, see the new PR. Sorry for any inconvenience, I'm "not really good" with git.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment