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

Valid finalizer by dmitriyse · Pull Request #532 · pythonnet/pythonnet · GitHub

/ pythonnet Public

Valid finalizer - #532

Closed
dmitriyse wants to merge 17 commits into
pythonnet:masterfrom
dmitriyse:valid-finalizer
ConversationCommits (17)ChecksFiles changed
Closed

Valid finalizer#532
dmitriyse wants to merge 17 commits into
pythonnet:masterfrom
dmitriyse:valid-finalizer

Conversation

dmitriyse commented Aug 28, 2017

Copy link
Copy Markdown
Contributor

What does this implement/fix? Explain your changes.

Implements really working finalizers solution.
...

Does this close any currently open issues?

Probably some issues will be fixed.
...

Any other comments?

Current finalizers solution are totally not working. Probably interop calls are disallowed in the C# finalizers. Or Finalizer thread becomes broken after a few calls to ~PyObject.
...

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 28, 2017

Copy link
Copy Markdown

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

codecov Bot commented Aug 28, 2017 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #532 into master will increase coverage by 0.03%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
+ Coverage   76.99%   77.03%   +0.03%     
==========================================
  Files          64       65       +1     
  Lines        5612     5747     +135     
  Branches      888      911      +23     
==========================================
+ Hits         4321     4427     +106     
- Misses       1002     1016      +14     
- Partials      289      304      +15
Flag Coverage Δ
#setup_linux 69.42% <ø> (ø) ⬆️
#setup_windows 76.23% <72.22%> (+0.05%) ⬆️
Impacted Files Coverage Δ
src/runtime/debughelper.cs 0% <ø> (ø) ⬆️
src/runtime/interop.cs 95.71% <ø> (-0.03%) ⬇️
src/runtime/runtime.cs 91.6% <100%> (+0.62%) ⬆️
src/runtime/classmanager.cs 94.9% <100%> (+0.06%) ⬆️
src/runtime/typemanager.cs 84.16% <100%> (+0.13%) ⬆️
src/runtime/classderived.cs 88.41% <100%> (+0.07%) ⬆️
src/runtime/methodbinding.cs 80% <100%> (ø) ⬆️
src/runtime/moduleobject.cs 85% <100%> (-0.06%) ⬇️
src/runtime/genericutil.cs 86.36% <100%> (+0.42%) ⬆️
src/runtime/assemblymanager.cs 89.83% <100%> (+0.05%) ⬆️
... and 11 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 c18285f...5ffab45. Read the comment docs.

dmitriyse force-pushed the valid-finalizer branch 10 times, most recently from 0212233 to bfb6e59 Compare August 29, 2017 16:19
Runtime/Shutdown loop stores old caches and static variables. It's pr…
9de6f7b 
…oduces bugs when CPython freeing up enough objects.
dmitriyse force-pushed the valid-finalizer branch 17 times, most recently from be8154f to 50fb157 Compare September 1, 2017 13:33
dmitriyse mentioned this pull request Sep 12, 2017
Closed
den-run-ai reviewed Sep 12, 2017
View reviewed changes
Comment thread src/runtime/pyobject.cs Outdated
}
}
}
catch

den-run-ai Sep 12, 2017

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

isn't is better to explicitly fail, instead of "bypassing" the exception handling with an empty catch {}? What issue is this solving that disposed and disposing flags are not taking care of?

den-run-ai reviewed Sep 12, 2017
View reviewed changes
Comment thread src/runtime/pyreferencedecrementer.cs

namespace Python.Runtime
{
internal class PyReferenceDecrementer : IDisposable

den-run-ai Sep 12, 2017

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 explain (in the comments) the purpose of PyReferenceDecrementer and what issues it addressed?

den-run-ai reviewed Sep 12, 2017
View reviewed changes
Comment thread src/runtime/pythonengine.cs
////Marshal.FreeHGlobal(_programName);
////_programName = IntPtr.Zero;
////Marshal.FreeHGlobal(_pythonPath);
////_pythonPath = IntPtr.Zero;

den-run-ai Sep 12, 2017

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

💯 👍

den-run-ai reviewed Sep 12, 2017
View reviewed changes
Comment thread src/runtime/pythonengine.cs Outdated

protected virtual void Dispose(bool disposing)
{
//ReleeaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock.

den-run-ai Sep 12, 2017

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

typo here

den-run-ai mentioned this pull request Sep 14, 2017
Closed

den-run-ai commented Sep 21, 2017

Copy link
Copy Markdown
Contributor

@dmitriyse @filmor i left some comments here, but I generally approve this pull request:

#532 (comment)
#532 (comment)
#532 (comment)

den-run-ai suggested changes Sep 21, 2017
View reviewed changes

den-run-ai left a comment

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 resolve merge conflicts

den-run-ai mentioned this pull request Nov 24, 2017
Closed
dmitriyse mentioned this pull request Jun 24, 2018
Merged
4 tasks

den-run-ai commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

@dmitriyse can you please resolve one merge conflict? can you remind me the order of PRs? If I don't get any feedback from @filmor @vmuriart @yagweb, then I'm going review again and merge this within 2 weeks, when I plan to be on vacation.

den-run-ai requested a review from yagweb June 24, 2018 19:54

den-run-ai commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

@dmitriyse ok, i found this message from you, please let me know if this is still valid:

#532 also depends on #534, (which is only a bunch of general fixes).
#532 is a superset of #534. So #534 just helps to separate general fixes from finalizer changes.
#531 does not depends on anything.

dmitriyse commented Jun 25, 2018

Copy link
Copy Markdown
Contributor Author

I will answer some day latter. Sorry.

filmor added this to the 2.4.0 milestone Aug 30, 2018
filmor mentioned this pull request Jan 18, 2019
Closed
filmor removed this from the 2.4.0 milestone Feb 5, 2019
filmor closed this Apr 1, 2019
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 Awaiting requested review from filmor
tonyroberts Awaiting requested review from tonyroberts
vmuriart Awaiting requested review from vmuriart
yagweb Awaiting requested review from yagweb
1 more reviewer
den-run-ai den-run-ai requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL