Skip to content
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

Use weakref finalize to close visa connections #5341

Merged
merged 12 commits into from
Sep 12, 2023

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Sep 3, 2023

Partial fix for #3774 it's tricky to do this generally since the finalizer cannot take a reference to the instrument it self and therefore also not a method on this such as self.close.

For visa instruments this is possible to implement since we only need a reference to the visa_handle to close the connection. This could also be implemented more generally if we created a handle/closer class on all instruments.

This also adds tests that confirms that visa instruments are closed once the instrument is out of scope and gc'ed.

Closes #5356 by closing the resource_manager in close if the backend is sim. Since this only applies to simulated instruments we don't add it to the finalizer but assume as always that a test must cleanup after it self.

@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Merging #5341 (a033320) into master (43560ec) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5341      +/-   ##
==========================================
+ Coverage   67.62%   67.63%   +0.01%     
==========================================
  Files         360      360              
  Lines       30059    30069      +10     
==========================================
+ Hits        20328    20338      +10     
  Misses       9731     9731              

qcodes/tests/test_visa.py Outdated Show resolved Hide resolved
@jenshnielsen jenshnielsen marked this pull request as ready for review September 5, 2023 08:52
Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and a newsfragment? :)

qcodes/instrument/visa.py Show resolved Hide resolved
qcodes/instrument/visa.py Outdated Show resolved Hide resolved
qcodes/tests/test_visa.py Outdated Show resolved Hide resolved
@jenshnielsen jenshnielsen added this pull request to the merge queue Sep 12, 2023
Merged via the queue into microsoft:master with commit 692a2e6 Sep 12, 2023
@jenshnielsen jenshnielsen deleted the shutdown_visa branch September 12, 2023 14:13
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.

Pyvisa sim instruments are not cleanup on close.
2 participants