Valid finalizer #532

dmitriyse · 2017-08-28T14:47:19Z

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 · 2017-08-28T14:47:21Z

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

codecov · 2017-08-28T19:29:18Z

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.

…oduces bugs when CPython freeing up enough objects.

den-run-ai · 2017-09-12T19:53:14Z

src/runtime/pyobject.cs

+                            }
+                        }
+                    }
+                    catch


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 · 2017-09-12T20:15:39Z

src/runtime/pyreferencedecrementer.cs

+
+namespace Python.Runtime
+{
+    internal class PyReferenceDecrementer : IDisposable


please explain (in the comments) the purpose of PyReferenceDecrementer and what issues it addressed?

den-run-ai · 2017-09-12T20:23:32Z

src/runtime/pythonengine.cs

+                ////Marshal.FreeHGlobal(_programName);
+                ////_programName = IntPtr.Zero;
+                ////Marshal.FreeHGlobal(_pythonPath);
+                ////_pythonPath = IntPtr.Zero;


den-run-ai · 2017-09-12T20:24:00Z

src/runtime/pythonengine.cs

@@ -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.


den-run-ai · 2017-09-21T14:16:44Z

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

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

den-run-ai

Please resolve merge conflicts

…. Review fixes.

den-run-ai · 2018-06-24T19:54:28Z

@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 · 2018-06-24T20:10:28Z

@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 · 2018-06-25T06:21:22Z

I will answer some day latter. Sorry.

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

Stability tests improved, compile warning fixed.

6c1abfa

dmitriyse mentioned this pull request Sep 12, 2017

memory leak - simple function call in a loop #541

Closed

den-run-ai reviewed Sep 12, 2017

View reviewed changes

den-run-ai requested review from filmor, tonyroberts and vmuriart September 13, 2017 03:15

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

.NET Core support and CoreCLR embedding - cross-platform API #96

Closed

den-run-ai approved these changes Sep 21, 2017

View reviewed changes

den-run-ai suggested changes Sep 21, 2017

View reviewed changes

dmitriyse force-pushed the valid-finalizer branch from 82c19b6 to 8a107f3 Compare September 22, 2017 01:39

dse added 2 commits October 10, 2017 14:45

Update TestRuntime.cs

507edc9

Upgrading NUnit to 3.8.1 for better mono stability and R# support.

e86405d

dmitriyse force-pushed the valid-finalizer branch from 8a107f3 to 37c2449 Compare November 16, 2017 22:00

den-run-ai mentioned this pull request Nov 24, 2017

Memory Leak in python when using a C# DLL #577

Closed

Finalizer implementation improvement. PythonException memory leak fix…

80ff754

…. Review fixes.

dmitriyse force-pushed the valid-finalizer branch from 7fd6fc4 to 80ff754 Compare January 12, 2018 18:36

Merge branch 'master' into valid-finalizer

5ffab45

dmitriyse mentioned this pull request Jun 24, 2018

PyObject finalizer #692

Merged

4 tasks

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

filmor added this to the 2.4.0 milestone Aug 30, 2018

filmor mentioned this pull request Jan 18, 2019

Release 2.4.0 #509

Closed

filmor removed this from the 2.4.0 milestone Feb 5, 2019

filmor closed this Apr 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Valid finalizer #532

Valid finalizer #532

dmitriyse commented Aug 28, 2017

mention-bot commented Aug 28, 2017

codecov bot commented Aug 28, 2017 •

edited

Loading

den-run-ai Sep 12, 2017

den-run-ai Sep 12, 2017

den-run-ai Sep 12, 2017

den-run-ai Sep 12, 2017

den-run-ai commented Sep 21, 2017

den-run-ai left a comment

den-run-ai commented Jun 24, 2018

den-run-ai commented Jun 24, 2018

dmitriyse commented Jun 25, 2018

Valid finalizer #532

Valid finalizer #532

Conversation

dmitriyse commented Aug 28, 2017

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

mention-bot commented Aug 28, 2017

codecov bot commented Aug 28, 2017 • edited Loading

Codecov Report

den-run-ai Sep 12, 2017

Choose a reason for hiding this comment

den-run-ai Sep 12, 2017

Choose a reason for hiding this comment

den-run-ai Sep 12, 2017

Choose a reason for hiding this comment

den-run-ai Sep 12, 2017

Choose a reason for hiding this comment

den-run-ai commented Sep 21, 2017

den-run-ai left a comment

Choose a reason for hiding this comment

den-run-ai commented Jun 24, 2018

den-run-ai commented Jun 24, 2018

dmitriyse commented Jun 25, 2018

codecov bot commented Aug 28, 2017 •

edited

Loading