Skip to content

feature: replace __del__ with weakref.finalize()#4386

Closed
maximilianluc wants to merge 1 commit intomicrosoft:masterfrom
maximilianluc:feature/weakref-finalize
Closed

feature: replace __del__ with weakref.finalize()#4386
maximilianluc wants to merge 1 commit intomicrosoft:masterfrom
maximilianluc:feature/weakref-finalize

Conversation

@maximilianluc
Copy link
Copy Markdown
Contributor

Feature addition: replacing del with weakref.finalize() as called method during object garbage collection.

Issue: #3774

  • renamed __del__ as __finalize, logic stays the same.
  • added weakref.finalize() in __init__ in instrument.py calling upon '__finalize()' during gc.

@maximilianluc
Copy link
Copy Markdown
Contributor Author

maximilianluc commented Jul 12, 2022

I think we can rename __del__ to __finalize to be called as weakref.finalize(obj, __finalize), right? With the logic in __del__ , now in __finalize(), the close() method should stay alive until the instrument is gc'd. Or am I overlooking something?

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 12, 2022

Codecov Report

Merging #4386 (a450af6) into master (a450af6) will not change coverage.
The diff coverage is n/a.

❗ Current head a450af6 differs from pull request most recent head 49d7082. Consider uploading reports for the commit 49d7082 to get more accurate results

@@           Coverage Diff           @@
##           master    #4386   +/-   ##
=======================================
  Coverage   68.09%   68.09%           
=======================================
  Files         299      299           
  Lines       31535    31535           
=======================================
  Hits        21473    21473           
  Misses      10062    10062           

@jenshnielsen
Copy link
Copy Markdown
Collaborator

@maximilianluc Thanks for this. It looks great but I will need a little bit of time to test this manually to see how it works in practice

@jenshnielsen jenshnielsen force-pushed the feature/weakref-finalize branch from 32a0ee5 to 52f080f Compare August 26, 2022 09:04
@jenshnielsen jenshnielsen force-pushed the feature/weakref-finalize branch from 52f080f to 49d7082 Compare September 2, 2022 07:59
@jenshnielsen
Copy link
Copy Markdown
Collaborator

@maximilianluc Sorry for the late response. I have spent a bit of time investigating this and unfortunately its a bit more complicated as the finalize method cannot contain a reference to self since that would increase the ref count to the instrument preventing the finalizer to be called. https://docs.python.org/3/library/weakref.html#weakref.finalize

In #5341 I have implemented a partial solution for visa instruments that do not have this issue

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.

2 participants