- Notifications
You must be signed in to change notification settings - Fork752
Fix Re-initialization#343
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedJan 30, 2017 • edited by codecovbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by codecovbot
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
Continue to review full report at Codecov.
|
Is this branch contain the same changes from#341? I'm reading it from my phone so hard to tell. |
To answer my own question, it is. I'm testing this now but its looking good! |
Failing on Appveyor Py33 https://ci.appveyor.com/project/vmuriart/pythonnet/build/fix-shutdown-517/job/adnsiyd4278ypy1m |
@filmor you ok with me rebasing your work ontop of the master branch and push it here? |
| throw new PythonException(); | ||
| } | ||
| return new PyObject(result); |
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.
The return within thetry statement can get tricky.
http://stackoverflow.com/questions/3216046/does-the-c-sharp-finally-block-always-execute
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.
I don't really see the issue here.
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.
To elaborate, the link that you posted warns against relying ontry {} finally {} as thefinally may not be executed, but in such a case the problem has either exited already or is in the process of doing so, so we don't have to worry about dereferencing the Python resources anyhow.
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.
It was more of a caveat, return statements withintry statements confuse me at times.
7e26298 to3e1a313Comparesrc/runtime/importhook.cs Outdated
| Runtime.XDecref(py_import); | ||
| } | ||
| } | ||
| internal static void Cleanup() |
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.
Is there a call toCleanup missing?
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.
That part is not used anymore as there is only a single ModuleDef instance that is kept for the whole run time, I'll remove it.
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.
Cool, after it's removed I'm ok merging it. This fixes the issue for Python2.7, but it still occassionally fails on Python3 but it seems to be due to a different section of code
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.
Yeah, I noticed that too, I'll try to reproduce the issue, for me it happens (after these patches) not anymore on Shutdown but only on Initialize.
Keeping the PyMethodDef around like the documentation(https://docs.python.org/3.6/c-api/module.html#c.PyModuleDef) suggestsfixes issuepythonnet#262.
Cool, will merge after CI completes. Thanks! |
What does this implement/fix? Explain your changes.
Fixes issue#262 by countering two memory corruption issues.
Does this close any currently open issues?
#262.
Any other comments?
Is currently based on my other PR, but doesn't strictly need it, I could rebase.