Conversation
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.
The return within the try statement can get tricky.
http://stackoverflow.com/questions/3216046/does-the-c-sharp-finally-block-always-execute
There was a problem hiding this comment.
I don't really see the issue here.
There was a problem hiding this comment.
To elaborate, the link that you posted warns against relying on try {} finally {} as the finally 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.
It was more of a caveat, return statements within try statements confuse me at times.
filmor
force-pushed
the
fix-shutdown
branch
2 times, most recently
from
7e26298 to
3e1a313
Compare
src/runtime/importhook.cs
Outdated
| } | ||
| } | ||
|
|
||
| internal static void Cleanup() |
There was a problem hiding this comment.
Is there a call to Cleanup missing?
There was a problem hiding this comment.
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.
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.
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) suggests fixes issue pythonnet#262.
|
Cool, will merge after CI completes. Thanks! |
3 participants
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.