FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixed Second PythonEngine.Initialize call, all sensitive static variables now reseted. by dmitriyse · Pull Request #534 · pythonnet/pythonnet · GitHub

/ pythonnet Public

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

Merged
filmor merged 8 commits into
pythonnet:masterfrom
dmitriyse:valid-engine-reset
Nov 14, 2018
ConversationCommits (8)ChecksFiles changed
Merged

Fixed Second PythonEngine.Initialize call, all sensitive static variables now reseted.#534
filmor merged 8 commits into
pythonnet:masterfrom
dmitriyse:valid-engine-reset

Conversation

dmitriyse commented Aug 31, 2017 •
edited by filmor
Loading

Copy link
Copy Markdown
Contributor

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 commented Aug 31, 2017

Copy link
Copy Markdown

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

codecov Bot commented Aug 31, 2017 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@            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 force-pushed the valid-engine-reset branch 6 times, most recently from 27905c8 to f205d56 Compare September 6, 2017 18:49
dmitriyse mentioned this pull request Sep 22, 2017
Closed
4 tasks
den-run-ai reviewed Feb 10, 2018
View reviewed changes
Comment thread CHANGELOG.md Outdated
### 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.

den-run-ai Feb 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

please include reference to this PR

den-run-ai mentioned this pull request Jun 24, 2018
Closed
4 tasks

den-run-ai commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

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

dmitriyse commented Jun 25, 2018

Copy link
Copy Markdown
Contributor Author

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

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

filmor commented Oct 29, 2018

Copy link
Copy Markdown
Member

@denfromufa Could you review this again?

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

den-run-ai commented Nov 14, 2018

Copy link
Copy Markdown
Contributor

@filmor i don't see any obvious issues.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

filmor filmor approved these changes
+1 more reviewer
den-run-ai den-run-ai left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL