rmthread should be cleaned up when the last observer is removed

[merrickheley]

https://github.com/LudovicRousseau/pyscard/blob/55fc8f1ac84f04197959f80592eef770fa8bde34/smartcard/CardMonitoring.py#L116

Currently when the last observer is removed the reference to rmthread is dropped:

if self.countObservers() == 0:
    if self.rmthread != None:
        self.rmthread = None

Should the thread not be stopped and joined if it is no longer needed? A suggested fix would be:

if self.countObservers() == 0:
    if self.rmthread != None:
        self.rmthread.stop()
        self.rmthread.join()
        self.rmthread = None

I'd be happy to make a pull request with this fix if this change is acceptable.

[LudovicRousseau]

Your patch looks correct.

I do not use CardMonitor() myself. Do you have a sample code that exhibits the bug and shows that your change fixes the problem?

[merrickheley]

import threading
import time

import smartcard.CardMonitoring

smartcard.CardMonitoring._START_ON_DEMAND_ = True

if __name__ == "__main__":
    # a simple card observer that prints added/removed cards
    class printobserver(smartcard.CardMonitoring.CardObserver):

        def __init__(self, obsindex):
            self.obsindex = obsindex

        def update(self, observable, handlers):
            addedcards, removedcards = handlers
            print("%d - added:   %s" % (self.obsindex, str(addedcards)))
            print("%d - removed: %s" % (self.obsindex, str(removedcards)))

    def show_running_threads():
        for thread in threading.enumerate():
            print("found", thread)

    def run_test(testnum):
        observer = printobserver(testnum)
        readermonitor.addObserver(observer)
        show_running_threads()
        time.sleep(3)
        readermonitor.deleteObserver(observer)

    readermonitor = smartcard.CardMonitoring.CardMonitor()
    run_test(1)

    print("-"*80)
    print("Deleting last observer. There is now nothing using the cardmonitor, but the thread still exists")
    show_running_threads()
    print("-"*80)

    run_test(2) 

Output without fix:

found <_MainThread(MainThread, started 139950329587456)>
found <__CardMonitoringThreadSingleton(Thread-1, started daemon 139950185875200)>
1 - added:   []
1 - removed: []
--------------------------------------------------------------------------------
Deleting last observer. There is now nothing using the cardmonitor, but the thread still exists
found <_MainThread(MainThread, started 139950329587456)>
found <__CardMonitoringThreadSingleton(Thread-1, started daemon 139950185875200)>
--------------------------------------------------------------------------------
found <_MainThread(MainThread, started 139950329587456)>
found <__CardMonitoringThreadSingleton(Thread-1, started daemon 139950185875200)>

Output with fix:

found <_MainThread(MainThread, started 140650581600000)>
found <__CardMonitoringThreadSingleton(Thread-1, started daemon 140650437887744)>
1 - added:   [3B F8 13 00 00 81 31 FE 15 59 75 62 69 6B 65 79 34 D4 / Yubico Yubikey 4 OTP+U2F+CCID 01 00]
1 - removed: []
--------------------------------------------------------------------------------
Deleting last observer. 
found <_MainThread(MainThread, started 140650581600000)>
--------------------------------------------------------------------------------
found <_MainThread(MainThread, started 140650581600000)>
found <__CardMonitoringThreadSingleton(Thread-32, started daemon 140650437887744)>
2 - added:   []
2 - removed: []

Note there is slightly different behaviour here, the Observer gets a callback as though it's been run for the first time.

I had to make a slight change to the CardMonitoringThread to get this working as well:

    def join(self, *args, **kwargs):
        if self.instance:
            self.instance.join(*args, **kwargs)
            CardMonitoringThread.instance = None

I'll put up a pull request if you're happy with this behaviour.