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

Close handle on signalEvent #9838

Merged
merged 2 commits into from
Jan 4, 2019
Merged

Conversation

wenhaochen
Copy link
Contributor

@wenhaochen wenhaochen commented Dec 31, 2018

hi!
signalEvent not be closed, handle leak might happen when calling Open()func a lot,
it seems not hard to recurrent such issue,
and as discuss on https://discuss.elastic.co/t/signalevent-not-be-closed/157966
signalEvent actually not work as same function in microsoft docs,
so I just add "defer" here to free the handle, which works in my test env

I'm confused that is it necessary to add any test cases or comments for the Fix, since it's sample ...
thanks !

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@wenhaochen wenhaochen requested a review from a team as a code owner December 31, 2018 17:14
@wenhaochen
Copy link
Contributor Author

I got the CLA, any latency?

@ruflin
Copy link
Member

ruflin commented Jan 1, 2019

jenkins, test this

@ruflin
Copy link
Member

ruflin commented Jan 1, 2019

@andrewkroh Could you take a look?
@wenhaochen Could you add an entry to the changelog.next file?

@urso urso removed the request for review from a team January 2, 2019 15:19
@andrewkroh
Copy link
Member

@wenhaochen Could you please rebase this on master to pull in some fixes that will make the CI testing start working on this PR. Assuming the tests continue to pass then this LGTM. Thanks.

@wenhaochen
Copy link
Contributor Author

@andrewkroh I have rebased on master, but still failed with test cases in auditbeat and metricbeat

@wenhaochen
Copy link
Contributor Author

I have tried twice, passed eventually.

@andrewkroh
Copy link
Member

jenkins, test this

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@andrewkroh andrewkroh merged commit 5b1e4cd into elastic:master Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants