Skip to content

Make finalizer use a task #852

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

Conversation

Martin-Molinero
Copy link
Contributor

@Martin-Molinero Martin-Molinero commented Apr 23, 2019

After
imagen

Before
imagen

- The callback set by `Runtime.Py_AddPendingCall()` was not being
triggered in some cases in a multithreading environment. Replacing it
with a `Task`
@filmor
Copy link
Member

filmor commented Apr 24, 2019

Great, thanks for your continuous efforts to backport fixes, it's highly appreciated. I'll look into merging this before the 2.4 release :)

@codecov
Copy link

codecov bot commented Apr 27, 2019

Codecov Report

Merging #852 into master will decrease coverage by 11.57%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #852       +/-   ##
==========================================
- Coverage   76.88%   65.3%   -11.58%     
==========================================
  Files          64       1       -63     
  Lines        5935     294     -5641     
  Branches      976       0      -976     
==========================================
- Hits         4563     192     -4371     
+ Misses       1041     102      -939     
+ Partials      331       0      -331
Flag Coverage Δ
#setup_linux 65.3% <ø> (ø) ⬆️
#setup_windows ?
Impacted Files Coverage Δ
setup.py 65.3% <0%> (-21.77%) ⬇️
src/runtime/pyansistring.cs
src/runtime/constructorbinder.cs
src/runtime/methodbinder.cs
src/runtime/Util.cs
src/runtime/delegatemanager.cs
src/runtime/eventbinding.cs
src/runtime/pyiter.cs
src/runtime/pynumber.cs
src/runtime/interop27.cs
... and 53 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 6034640...cf17b3c. Read the comment docs.

@filmor
Copy link
Member

filmor commented Apr 29, 2019

The Linux build seems to be fine apart from an unrelated error (happens on master as well). On Windows, however, for some reason this seems to stall, the builds time out during the unit tests.

@filmor
Copy link
Member

filmor commented Apr 29, 2019

It trips up the DomainReloadAndGC test, it looks like there is a deadlock in DisposeAll.

@filmor
Copy link
Member

filmor commented Apr 29, 2019

If I remove the GIL call from DisposeAll everything works fine as far as I can tell right now.

@filmor filmor merged commit 33db56d into pythonnet:master Apr 30, 2019
@filmor
Copy link
Member

filmor commented Apr 30, 2019

Great, this looks a lot better, thank you very much :)

@amos402
Copy link
Member

amos402 commented Jun 14, 2019

@Martin-Molinero Using Py_AddPendingCall is aiming for no Python thread, although we called PyEval_InitThreads at Initialize, but that is a optimize point that we can prevent create a GIL on single thread environment. Also using Py.GIL may create a temporary Python thread state which takes overhead.
I think we can make a better user experience for the finalizer, we can setup a timer for finalizing queue, if the time past threshold value then create a Python thread to dispose them. That may also help for the queue size don't reach count threshold for a long time but keep the object reference.

@Martin-Molinero Martin-Molinero deleted the fix-memory-leak-finalizer-use-task branch August 27, 2019 20:40
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.

3 participants