Skip to content

Fixed Second PythonEngine.Initialize call, all sensitive static variables now reseted. #534

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

Merged
merged 8 commits into from
Nov 14, 2018

Conversation

dmitriyse
Copy link
Contributor

@dmitriyse dmitriyse commented Aug 31, 2017

What does this implement/fix? Explain your changes.

This change fixes hidden bug. Once python cleaning up enough memory, objects from previous engine run becomes corrupted and produces segfault.
...

Does this close any currently open issues?

It's required but not enough to fix this issue
#499
...

Any other comments?

Described problem never pop up, because ~PyObject currently does not called by the GC (due to some reasons). So many many time pythonnet lives without garbage collection with constant memory leak.
...

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@mention-bot
Copy link

@dmitriyse, thanks! @vmuriart, @tonyroberts, @cgohlke, @tiran and @hsoft, please review this.

@codecov
Copy link

codecov bot commented Aug 31, 2017

Codecov Report

Merging #534 into master will increase coverage by 8.64%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   68.55%   77.19%   +8.64%     
==========================================
  Files           1       63      +62     
  Lines         283     5754    +5471     
  Branches        0      907     +907     
==========================================
+ Hits          194     4442    +4248     
- Misses         89     1013     +924     
- Partials        0      299     +299
Flag Coverage Δ
#setup_linux 69.42% <ø> (+0.87%) ⬆️
#setup_windows 76.39% <100%> (?)
Impacted Files Coverage Δ
src/runtime/pythonengine.cs 80.35% <ø> (ø)
src/runtime/moduleobject.cs 82.79% <100%> (ø)
src/runtime/runtime.cs 88.65% <100%> (ø)
src/runtime/assemblymanager.cs 89.17% <100%> (ø)
src/runtime/genericutil.cs 86.36% <100%> (ø)
src/runtime/typemanager.cs 82.77% <100%> (ø)
src/runtime/classderived.cs 88.41% <100%> (ø)
src/runtime/classmanager.cs 94.9% <100%> (ø)
src/runtime/pyscope.cs 57.92% <100%> (ø)
src/runtime/pyansistring.cs 100% <0%> (ø)
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af394ee...dbd5fdd. Read the comment docs.

@dmitriyse dmitriyse force-pushed the valid-engine-reset branch 6 times, most recently from 27905c8 to f205d56 Compare September 6, 2017 18:49
CHANGELOG.md Outdated
@@ -21,6 +21,8 @@ This document follows the conventions laid out in [Keep a CHANGELOG][].

### Fixed

- Fixed secondary PythonEngine.Initialize call, all sensitive static variables now reseted.
This is a hidden bug. Once python cleaning up enough memory, objects from previous engine run becomes corrupted.
Copy link
Contributor

Choose a reason for hiding this comment

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

please include reference to this PR

@den-run-ai den-run-ai mentioned this pull request Jun 24, 2018
4 tasks
@den-run-ai
Copy link
Contributor

@dmitriyse can you please resolve a conflict in this PR?

@dmitriyse
Copy link
Contributor Author

Yes, I will try to do it on this week.

@filmor filmor added this to the 2.4.0 milestone Jul 23, 2018
@filmor filmor modified the milestone: 2.4.0 Oct 18, 2018
@filmor filmor added the next label Oct 18, 2018
@filmor filmor force-pushed the valid-engine-reset branch from 0345af2 to 257dcc6 Compare October 19, 2018 15:00
@filmor
Copy link
Member

filmor commented Oct 29, 2018

@denfromufa Could you review this again?

@filmor filmor force-pushed the valid-engine-reset branch from 2145bf2 to 44d606b Compare November 14, 2018 13:38
@filmor filmor force-pushed the valid-engine-reset branch from 44d606b to dbd5fdd Compare November 14, 2018 13:39
@filmor filmor merged commit 88d61a9 into pythonnet:master Nov 14, 2018
@den-run-ai
Copy link
Contributor

@filmor i don't see any obvious issues.

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