usb1: Clean up poll fd finalizer

[maxtruxa]

There is a bug in usb1.USBContext.setPollFDNotifiers() that makes it impossible to be used correctly. During cleanup you always hit an assert.

Minimal repro:

# minimal setup
python3 -m virtualenv libusb-repro-venv
. libusb-repro-venv/bin/activate
python3 -m pip install libusb1
# actual repro
python3 - <<'EOS'
from usb1 import USBContext
with USBContext() as ctx:
    ctx.setPollFDNotifiers()
EOS

Resulting stacktrace:

Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "/home/maxtruxa/libusb-repro-venv/lib/python3.9/site-packages/usb1/__init__.py", line 2147, in __exit__
    self.close()
  File "/home/maxtruxa/libusb-repro-venv/lib/python3.9/site-packages/usb1/__init__.py", line 2203, in close
    self.__close()
  File "/usr/lib/python3.9/weakref.py", line 580, in __call__
    return info.func(*info.args, **(info.kwargs or {}))
  File "/home/maxtruxa/libusb-repro-venv/lib/python3.9/site-packages/usb1/__init__.py", line 2235, in ___close
    assert handle not in finalizer_dict
AssertionError

setPollFDNotifiers() registers a finalizer, but the finalizer does not remove itself from the finalizer_dict. This PR resolves the issue following the same pattern that was used in USBTranser.__init__(), USBDeviceHandle.__init__() and USBDevice.__init__() to solve the exact same problem.

[vpelletier]

Thanks for identifying this bug.

I think I see a reference loop in the fix, unfortunately: USBContext.__unregisterFinalizer is a regular method (not static), so self.__unregisterFinalizer holds a reference to self. This bound method is then referenced by the partial object used as the unregisterFinalizer argument for __finalizePollFDNotifiers, which is itself an argument to the finalizer to trigger upon self deallocation - so the finalizer holds a reference to the object it is watching.

In other uses, the watched object should never be the one holding the finalizer mapping, which should avoid such reference loop. This is also why there are all those "Note: staticmethod" comments everywhere.

So while this change is in the right direction, I suspect it could be because the finalizer is never called as the reference loop would keep it alive. This is probably not the whole story, as IIRC the python GC has the ability to break reference loops, and then I do not know what happens to finalizers.

I think the following minor change to your fix should allow breaking the loop: c548206f272025349eddf46b9e3d894c36f01591 (pushed to a temporary branch).

Could you test with this change if this does not cause regressions ? If everything is fine, please feel free to squash my commit into your and push the result to this merge request.

[maxtruxa]

It appears to still work with the change. Instead of passing two parameters I opted into bundling them up in a lambda and keeping the unregisterFinalizer argument to the finalize function. This is consistent with the existing code.

[vpelletier]

TL;DR: could you replace the lambda with a functools.partial ? Then I'll merge.

Now, in a lot more details, my train of thoughts. Bikeshed alert !

The reason why several finalizers take a callable as argument is because that finalizer is a method on another class (the class which created the one building the finalizer). Such methods touch protected/private members of their own instances, so such code does not belong on the class which creates the finalizer, and instead these methods are given to be passed on to the finalizer.

In the case of USBContext, there is no such other class: it is the root of the object tree as far as this module is concerned. I think there is no benefit in having part of the finalizing code in one place on the class and another part in another place, while I think it decreases readability. So consistency with non-USBContext implementation is not my priority (but passing a callable could be a good thing on its own merits). Note BTW that the other finalizer on USBContext, ___close, has no such callable argument.

Then again, because in this specific case the called code is quite simple, the difference is not much. Here are the alternatives I can identify which should work just as well, from the version I pushed to your current version:

My original suggestion, perhaps the least surprising:

                    handle=finalizer_handle,
                    finalizer_dict=self.__finalizer_dict,

Variant which only lets the finalizer access the pop method on finalizer dict, as it really does not need anything else:

                    handle=finalizer_handle,
                    finalizer_dict_pop=self.__finalizer_dict.pop,

Squashing both arguments by building a partial:

                    unregisterFinalizer=partial(self.__finalizer_dict.pop, finalizer_handle),

Your current version, using a lambda:

                finalizer_dict = self.__finalizer_dict
# ...
                    unregisterFinalizer=lambda: finalizer_dict.pop(finalizer_handle),

There is one thing I am worried about with a lambda, but could not reproduce in a few quick tries: I worry it could keep a reference to the functions' stack cell, which has a reference to self, which would cause a reference loop. It seems py3 (at least 3.11) only references what the lambda accesses and not the whole stack cell. There may be more factors influencing how the interpreter behaves here, overall I am not too confident in this lambda not causing issues later. Using a functools.partial (as you did in your original patch) would completely avoid such risk, and seems safer to me.

To be complete, I tend to have a slight preference for the finalizer_dict_pop version, as it does not require the creation of a functools.partial object (which is kind of redundant with weakref.finalizer's * and **). But I'm fine with either.