tivek commented 5 years ago

Fix a double close of GPIB connections which usually produces uncaught exceptions.

To reproduce the bug on pyvisa-py master:

$ ipython
Python 3.7.0 (default, Oct  9 2018, 10:31:47) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.1.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import visa                                                                                                                                                                                                                           

In [2]: rm = visa.ResourceManager('@py')                                                                                                                                                                                                      

In [3]: tek = rm.open_resource('GPIB0::30::INSTR')                                                                                                                                                                                            

In [4]: tek.query('*IDN?')                                                                                                                                                                                                                    
Out[4]: 'TEKTRONIX,TDS 3014C,0,CF:91.1CT FV:v4.05 TDS3GV:v1.00 TDS3FFT:v1.00 TDS3TRG:v1.00\n'

In [5]: quit()                                                                                                                                                                                                                                
libgpib: invalid descriptor
libgpib: invalid descriptor
Exception ignored in: <function Gpib.__del__ at 0x7f985e1996a8>
Traceback (most recent call last):
  File "/home/tivek/projects/linux-gpib-code/linux-gpib-user/language/python/Gpib.py", line 32, in __del__
gpib.GpibError: close() failed: One or more arguments to the function call were invalid.

Explanation and fix: GpibSession uses two Gpib.Gpib objects (linux-gpib project) to manage connections to the instrument and GPIB board. These clean up on their own in __del__() and we should not manually close their handles.

Long-term, __del__() should not be used for reliable finalization since it is a potentially ugly implementation detail of CPython's GC. Sadly, Gpib.Gpib does not expose a better interface for cleanup. At some point we should either patch/fork Gpib.Gpib or switch to classic gpib function calls with integer handles.

MatthieuDartiailh commented 5 years ago

Indeed this looks like a bad idea. At least the __del__ should be safe if the interface is closed. Could you implement the fix in gpib_ctypes and we could then switch to this implementation, since we do not win anything in using linux-gpib binding ?

MatthieuDartiailh commented 5 years ago

I will merge sometimes this week since this is no matter what the sensible solution for now.

tivek commented 5 years ago

I will release gpib_ctypes that has close() as you suggest.

However, I would not rule out using linux-gpib.Gpib. I've pushed a second commit which patches in a close() method. I somehow feel it is important to support the original Python GPIB bindings since it has the largest user base. In the mean time I can ping the linux-gpib project and see if they would be willing to add close().

tivek commented 5 years ago

linux-gpib has merged a patch that introduces Gpib.close() like gpib_ctypes 0.2.0. Until it gets released, and to support users of older linux-gpib releases, I propose to keep the monkeypatching code from 5968788.

MatthieuDartiailh commented 5 years ago

Sounds good. I agree that keeping the monkeypatch for now makes sense. Is this ready for merging ?

tivek commented 5 years ago

Ready for merging. Tested with linux-gpib r1767 (before the accepted patch), linux-gpib trunk as well as gpib_ctypes 0.2.0.

MatthieuDartiailh commented 5 years ago

bors r+

bors[bot] commented 5 years ago

Build succeeded

continuous-integration/travis-ci/push

MatthieuDartiailh commented 5 years ago

I forgot to ask and I am going to fix it right away but in the future please update the release notes.

tivek commented 5 years ago

Sorry about the release notes, will do next time.

MatthieuDartiailh commented 5 years ago

It is no big deal. I am just trying to keep the release notes up to date so that the release process is a bit more straightforward.

pyvisa / pyvisa-py

fix double close of GPIB interface and controller in GpibSession #171

Build succeeded