Cast libusb pollfd callbacks from null pointer to proper type

[whitequark]

This avoids an exception (sometimes a segfault) during Python process exit
with a signature like the following:

Exception ignored in: <bound method USBPoller.__del__ of <usb1.USBPoller object at 0x7f7700e4eda0>>
Traceback (most recent call last):
  File "/home/whitequark/.local/lib/python3.6/site-packages/usb1/__init__.py", line 1081, in __del__
  File "/home/whitequark/.local/lib/python3.6/site-packages/usb1/__init__.py", line 2127, in wrapper
  File "/home/whitequark/.local/lib/python3.6/site-packages/usb1/__init__.py", line 2387, in setPollFDNotifiers
ctypes.ArgumentError: argument 2: <class 'TypeError'>: expected CFunctionType instance instead of _ctypes.PyCSimpleType

I'm not sure how this ever worked--a ctypes change, perhaps? In any case, the POINTER(None) needs to be called before use, and cast to the callback type.

[vpelletier]

Looks good to me. I would like just one tiny change: POINTER(None)() seems to be the same as c_void_p(), and the latter looks a bit less surprising:

$ python
Python 2.7.15 (default, May  1 2018, 05:55:50)
[GCC 7.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from ctypes import *; POINTER(None)()
c_void_p(None)
$ python3
Python 3.6.6 (default, Jun 27 2018, 14:44:17)
[GCC 8.1.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from ctypes import *; POINTER(None)()
c_void_p(None)
$ pypy
Python 2.7.13 (6.0.0+dfsg-1, Apr 27 2018, 22:07:01)
[PyPy 6.0.0 with GCC 7.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>>> from ctypes import *; POINTER(None)()
c_void_p(None)

About why it works, I suspect user_data to never be used as if it is to contain python objects it would require complex casting. Maybe you are using it ?

Otherwise, it comes from the actual callbacks, and here I wonder if there would not be another issue in the code using python-libusb1 here: interpreter shutdown is extremely hard to get right (especially, __del__ may be called in any order at any time regardless of what still holds references to what), which is why several python-libusb1 classes are context managers. You may want to check that you are using these as such, or you may indeed encounter interpreter shutdown crashes. For example, the callback python callables could get garbage-collected before libusb is deinitialised. This would not explain why proposed patch would improve the situation, though.

In any case this change, modulo the c_void_p() simplification, looks good independently from any other issue.

[whitequark]

You may want to check that you are using these as such, or you may indeed encounter interpreter shutdown crashes. For example, the callback python callables could get garbage-collected before libusb is deinitialised. This would not explain why proposed patch would improve the situation, though.

Yes, I have indeed discovered this recently, which I don't think is visibly mentioned in the docs. I consider this a set of python-libusb1 bugs, especially since I can't easily restructure my code to look like you suggest.

[whitequark]

Patch updated.

[vpelletier]

I don't think is visibly mentioned in the docs

I tried to document it in the docstring of USBContext.open, although I did it after discovering and understanding the issue so it may not be the best way to describe or explain it. Suggestions very welcome.

I can't easily restructure my code to look like you suggest.

I know, I was very much annoyed when I was told unordered garbage collection is just how it works and not a bug, and that context managers were the only reliable solution (and their try..finally: close() equivalent).

There may be an alternative solution in rewriting python-libusb1 in C so that it can control garbage collection one level below the interpreter. But I have no intention of doing so, as then it would likely not work with pypy.

As an alternative to context management, note that you can also call <context>.close() before your program exits (so preferably in a try..finally block).

Patch updated.

You need to attach an instance of c_void_p to the class, again because of interpreter shutdown issues: module globals are deleted (replaced by None) before all instances in that module have been destroyed. This is why all (otherwise) globals which are involved in a destructor (and its callees) are all bound to classes, as __null_pointer was, despite how redundant and uggly it looks. Otherwise, on interpreter shutdown one will see None values popping here and there (here it would be NoneType is not callable).

Side note: in ctypes, None is supposed to work as a null pointer, so maybe this would happen to work in this case... I do not remember right now why I thought I had to prepare a null-pointer instance instead, and would not be surprised if ctypes was being a pain about how None is not a valid function pointer.

[whitequark]

There may be an alternative solution in rewriting python-libusb1 in C so that it can control garbage collection one level below the interpreter. But I have no intention of doing so, as then it would likely not work with pypy.

Right, I agree this would be undesirable. I'll try to fix these bugs when I hit them without major changes.

As an alternative to context management, note that you can also call .close() before your program exits (so preferably in a try..finally block).

Thanks, I think this will work.

You need to attach an instance of c_void_p to the class, again because of interpreter shutdown issues

Oh wow, I see now why GC is such a pain in this case, Python's del methods aren't like finalizers at all. This explains many things. Fixed.

[vpelletier]

Looks perfect, thanks for contributing !

[vpelletier]

I noticed only after the merge that cast needed to be bound to the class as well. I fixed that and released 1.6.5 .

[whitequark]

Thanks!

[whitequark]

As an alternative to context management, note that you can also call .close() before your program exits (so preferably in a try..finally block).

Incidentally, you were right; on Windows, unless I do this, my application reliably crashes with an access violation on every exit.