LudovicRousseau / pyscard

pyscard smartcard library for python
http://pyscard.sourceforge.net/
GNU Lesser General Public License v2.1
383 stars 110 forks source link

Configurable polling interval? #41

Open tycho opened 7 years ago

tycho commented 7 years ago

Can you please make the card monitoring thread's polling interval configurable?

My current hack works, but makes me sad:

        self._monitor = CardMonitor()
        self._monitor.addObserver(self)

        success = False
        attempts = 0
        sleeptime = 0.01
        while attempts < 5:
            try:
                self._monitor.rmthread.cardrequest.pcsccardrequest.pollinginterval = 5
                success = True
                break
            except:
                time.sleep(sleeptime)
                sleeptime = 0.1
            attempts += 1
        if success:
            print("Successfully set polling interval")
        else:
            print("Failed to set polling interval")

CardMonitoringThread's cardrequest field doesn't exist until the thread starts. So I have to wait before I can monkey-patch the polling interval value.

It'd be infinitely better if I could just set the polling interval when I create a CardMonitor instance, or if I could use a function call to set it (or both!)

LudovicRousseau commented 7 years ago

Active polling is bad and should not be used by PySCard. SCardGetStatusChange() should be used instead.

I do not use PCSCCardRequest() and I am not the author of this code.

I will have a look at the code. But I don't know when I will have time for that. So a patch to avoid polling is really welcome :-)

LudovicRousseau commented 7 years ago

I had a look at the waitforcard() and waitforcardevent() code https://github.com/LudovicRousseau/pyscard/blob/master/smartcard/pcsc/PCSCCardRequest.py#L108 The code is very complex.

My problem is that I am not sure of what the methods are supposed to do. The documentation is very limited. For example how to manage reader insertion or removal? Should the method just ignore readers added after the method started?

@tycho, why do you want to change the polling interval?

tycho commented 7 years ago

A fair question.

The app I'm using the library in has an option to minimize to the system tray. And while it's there, it doesn't need to interact with a smart card. The problem is that the pyscard ReaderMonitor and CardMonitor threads poll so frequently that the app will do nothing useful and eat away at battery life on laptops.

My first option was to clean up everything pyscard related and re-initialize it all when the app is restored from the tray. But then I started running into all kinds of weird crashy behavior that I couldn't explain. It turns out it's really hard to properly shut down and clean up all the pyscard stuff.

The second option is to leave the pyscard objects as they are, but make them poll less frequently to eat less CPU. So when my app is minimized to the tray, I set a really long poll interval, and when the app is restored, I restore the original short poll intervals.

LudovicRousseau commented 7 years ago

I do not plan/have time to rewrite ReaderMonitor and CardMonitor now. So a patch is welcome.

Maybe you can change your application to stop using ReaderMonitor and CardMonitor while in the system tray?

tycho commented 7 years ago

I changed it to stop using ReaderMonitor/CardMonitor at all, and that has helped greatly. The code I replaced it with:

https://github.com/tycho/yubioath-desktop/blob/master/yubioath/gui/ccid.py

On Tue, May 23, 2017 at 8:34 AM, Ludovic Rousseau notifications@github.com wrote:

I do not plan/have time to rewrite ReaderMonitor and CardMonitor now. So a patch is welcome.

Maybe you can change your application to stop using ReaderMonitor and CardMonitor while in the system tray?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LudovicRousseau/pyscard/issues/41#issuecomment-303437282, or mute the thread https://github.com/notifications/unsubscribe-auth/AABzsAaiMH2nipidwMHpj-dPqpIK9RDFks5r8vxzgaJpZM4Ne4RH .

ElouanPetereau commented 3 years ago

Hello,

I also add troubles with the threads polling speed while testing pyscard. I was using the PC/SC implementation for linux (pcsc-lite) and because of the fast polling speed, the pcsc daemon logs were completely filled with these calls making it unreadable.

After giving a look at the CardMonitor and ReaderMonitor code, I discovered that you can set the polling interval speed for the ReaderMonitor but not for the CardMonitor.

There is also a pollinginterval parameter for the cardrequest.pcsccardrequest object (this is the one that tycho was monkey-patching). This one is used by the cardrequest.waitforcard() and cardrequest.waitforcardevent() methods which call SCardGetStatusChanged() and loop using the pollinginterval only if an error is detected and if the timeout has not been reached. But because the sleep is done before checking this, the call will be at least cardrequest.pcsccardrequest.pollinginterval long.

The current CardMonitor implementation create cardrequest.pcsccardrequest object and loop with the waitforcardevent() method with a timeout of 0.1 seconds until a a specific call to stop the monitoring thread is done. Because the cardrequest.pcsccardrequest polling interval is 0.1 seconds and because in the waitforcardevent() methods the sleep is executed, then the call to SCardGetStatusChanged(), then the check for the end of the timeout, only one call is done thus changing cardrequest.pcsccardrequest.pollinginterval before calling waitforcardevent work. But the timeout have to be at equal or inferior to the pollinginterval parameter !

So, I think, a polling interval between each waitforcardevent() call inside the CardMonitor could be added to the object parameters to simplify its use. This polling interval will then be set to both the timeout and the polling interval of the cardrequest.pcsccardrequest object before calling waitforcardevent().

On top of that, I also discovered that when you create a CardMonitor object, a card monitoring thread is automatically created and thus polling calls are made even if no observer have been added to the monitor. I don't think this is the desired beahvior because the comments on the CardMonitoring.py file explain this:

note: a card monitoring thread will be running as long as the card monitor has observers, or CardMonitor.stop() is called. Do not forget to delete all your observers by calling deleteObserver, or your program will run forever...

This is due to the _START_ON_DEMAND_ variable in CardMonitoring.py being set to False by default. In Readermonitoring.py, I saw that the variable that as the same role (startOnDemand) is set to True by default which is, from what I understood, the required behaviour.

I made some changes to CardMonitor that makes it respect the behaviour described above and that allow you to edit the polling interval speed.I also decided to use a Borg Singleton implementation for the Cardmonitoring, like it is done for Readermonitoring, instead of the current "classic" Singleton implementation.

If you are interested in these changes, or if you want to add them to pyscard @LudovicRousseau, they are available on my fork of pyscard on the refactor_cardmonitor branch.

LudovicRousseau commented 3 years ago

I did not write the CardMonitor and ReaderMonitor code. Using an active loop with a low timeout value is a bad idea. On Windows and GNU/Linux we could use the special reader "\\?PnP?\Notification" to detect reader events. But the problem is that macOS does not support "\\?PnP?\Notification".

Thanks for the patches @ElouanPetereau. I may have a look but I don't know when.

ElouanPetereau commented 3 years ago

You're welcome.

And yes of course using CardMonitor and ReaderMonitor is not the efficient and cpu/battery friendly way to go. I only made these patches to reduce the problem.