Skip to content

(22878) Scope the call to CloseHandle#1999

Closed
mrwulf wants to merge 1 commit intopuppetlabs:masterfrom
mrwulf:master
Closed

(22878) Scope the call to CloseHandle#1999
mrwulf wants to merge 1 commit intopuppetlabs:masterfrom
mrwulf:master

Conversation

@mrwulf
Copy link
Contributor

@mrwulf mrwulf commented Oct 15, 2013

Without this patch the ensure block calls a private method (throwing an error). By continuing the scope used in the main block the correct method is called.

@puppetcla
Copy link

Waiting for CLA signature by @mrwulf

@mrwulf - 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.puppetlabs.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.puppetlabs.com/community/trivial_patch_exemption.html

@mrwulf
Copy link
Contributor Author

mrwulf commented Oct 15, 2013

This is a trivial change and doesn't require a cla.
On Oct 15, 2013 3:31 PM, "puppetcla" notifications@github.com wrote:

Waiting for CLA signature by @mrwulf https://github.com/mrwulf

@mrwulf https://github.com/mrwulf - 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.puppetlabs.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.puppetlabs.com/community/trivial_patch_exemption.html


Reply to this email directly or view it on GitHubhttps://github.com//pull/1999#issuecomment-26369475
.

@joshcooper
Copy link
Contributor

Thanks for the patch! The win32-process gem used to extend Windows::Handle from the windows-pr gem, so that the Process.CloseHandle method was available.

C:\work\win32-process>git checkout master
Branch master set up to track remote branch master from origin.
Switched to a new branch 'master'
C:\work\win32-process>ruby -r win32/process -e "Process.CloseHandle(0)"
C:\work\win32-process>

But newer version of the gem, based on ffi, and what we are shipping with puppet, doesn't do this:

C:\work\win32-process>git checkout ffi
Switched to branch 'ffi'
C:\work\win32-process>ruby -r win32/process -e "Process.CloseHandle(0)"
-e:1:in `': private method `CloseHandle' called for Process:Module (NoMethodError)

Being explicit about which CloseHandle we are using is a good thing™

@mrwulf
Copy link
Contributor Author

mrwulf commented Oct 16, 2013

Your test and explanation are spot on. Thanks-

I'm surprised this isn't affecting more people. Do you know when this
change will be merged?

Thanks for the patch! The win32-process gem used to extend
Windows::Handlehttps://github.com/djberg96/win32-process/blob/master/lib/win32/process.rb#L46from
the windows-pr gem, so that the Process.CloseHandle method was available.

C:\work\win32-process>git checkout master
Branch master set up to track remote branch master from origin.
Switched to a new branch 'master'
C:\work\win32-process>ruby -r win32/process -e "Process.CloseHandle(0)"
C:\work\win32-process>

But newer version of the gem, based on ffi, and what we are shipping with
puppet, doesn't do this:

C:\work\win32-process>git checkout ffi
Switched to branch 'ffi'
C:\work\win32-process>ruby -r win32/process -e "Process.CloseHandle(0)"
-e:1:in ': private methodCloseHandle' called for Process:Module
(NoMethodError)

Being explicit about which CloseHandle we are using is a good thing™


Reply to this email directly or view it on
GitHubhttps://github.com//pull/1999#issuecomment-26380485
.

@adrienthebo
Copy link
Contributor

summary: this was cherry-picked onto stable in 7e3493b and merged up into master; this should be released in 3.4.0. Thanks for the contribution!

@adrienthebo adrienthebo closed this Nov 8, 2013
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.

4 participants