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.

Sign up for GitHub

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

Jump to bottom

rmthread should be cleaned up when the last observer is removed #53

Closed
merrickheley opened this issue Nov 23, 2017 · 2 comments
Closed

rmthread should be cleaned up when the last observer is removed #53

merrickheley opened this issue Nov 23, 2017 · 2 comments

Comments

@merrickheley
Copy link
Contributor

pyscard/smartcard/CardMonitoring.py

Line 116 in 55fc8f1

self.rmthread = None

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
Copy link
Owner

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
Copy link
Contributor Author 

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.

merrickheley added a commit to merrickheley/pyscard that referenced this issue Nov 26, 2017
@merrickheley
Modify CardMonitoring's deleteObserver method to cleanly remove threads
1f07846 
This closes LudovicRousseau#53
This was referenced Nov 26, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@LudovicRousseau @merrickheley