- Notifications
You must be signed in to change notification settings - Fork750
Valid finalizer#532
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
Valid finalizer#532
Uh oh!
There was an error while loading.Please reload this page.
Conversation
mention-bot commentedAug 28, 2017
@dmitriyse, thanks!@vmuriart,@tonyroberts,@hsoft,@cgohlke and@tiran, please review this. |
codecovbot commentedAug 28, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
0212233 tobfb6e59Compare…oduces bugs when CPython freeing up enough objects.
be8154f to50fb157Comparesrc/runtime/pyobject.cs Outdated
| } | ||
| } | ||
| } | ||
| catch |
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.
isn't is better to explicitly fail, instead of "bypassing" the exception handling with an emptycatch {}? What issue is this solving thatdisposed anddisposing flags are not taking care of?
| namespace Python.Runtime | ||
| { | ||
| internal class PyReferenceDecrementer : IDisposable |
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.
please explain (in the comments) the purpose of PyReferenceDecrementer and what issues it addressed?
| ////Marshal.FreeHGlobal(_programName); | ||
| ////_programName = IntPtr.Zero; | ||
| ////Marshal.FreeHGlobal(_pythonPath); | ||
| ////_pythonPath = IntPtr.Zero; |
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.
💯 👍
src/runtime/pythonengine.cs Outdated
| @@ -568,15 +602,24 @@ internal GILState() | |||
| state = PythonEngine.AcquireLock(); | |||
| } | |||
| protected virtual void Dispose(bool disposing) | |||
| { | |||
| //ReleeaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock. | |||
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.
typo here
@dmitriyse@filmor i left some comments here, but I generally approve this pull request: |
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.
Please resolve merge conflicts
82c19b6 to8a107f3Compare@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. |
I will answer some day latter. Sorry. |
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.
AUTHORSCHANGELOG