Make YubiKeyDeviceListener resettable #89
Conversation
Currently, YubiKeyDeviceListener only exposes a singleton instance with no way for a consumer to reset it once it is started. The constructor is private, so the consumer has no option to use a private instance. This adds the ability for the consumer to reset the lazy-initialized singleton so that the listen thread doesn't stick around. Also, even though this class implemented IDisposable, most things that need to be in the Dispose method weren't there, including a way to stop the thread. The listen thread is now replaced by an asynchronous task that doesn't consume a system thread while it's not working.
Test Results: Windows 2 files 2 suites 2s ⏱️ Results for commit ce5ddc1. |
Test Results: Ubuntu 2 files 2 suites 5s ⏱️ Results for commit ce5ddc1. |
Test Results: MacOS 2 files 2 suites 1s ⏱️ Results for commit ce5ddc1. |
Minimum allowed line rate is |
There was a problem hiding this comment.
It looks good to me. Because of limited experience with this SDK, I don't feel confident in testing this PR myself. Perhaps @GregDomzalski will have some time to take a look ?
| private static readonly Lazy<YubiKeyDeviceListener> _lazyInstance = | ||
| new Lazy<YubiKeyDeviceListener>(() => new YubiKeyDeviceListener()); | ||
| /// <summary> | ||
| /// Disposes and closes the singleton instance of <see cref="YubiKeyDeviceListener"/>. |
There was a problem hiding this comment.
Summary: Instructs the SDK to stop listening for YubiKey events, effectively causing the SDK to go "dormant". Accessing Instance or calling YubiKeyDevice.FindAll() will restart the listener.
Remarks: Enumerating YubiKeys is actually done via a cache. As such, this cache must be maintained and kept up-to-date. This is done by starting several listening threads that run in the background. These listen for the relevant OS device arrival and removal events. Normally, these background listeners will run starting with the first enumeration call to the SDK and remain active until the process shuts down (i.e. a lazy singleton). But there are cases where you may not want the overhead of these listeners running all the time. While they do their best to not eat CPU, they can sometimes generate log noise, exceptions, etc. This method allows you to stop these background listeners and reclaim as much resources as possible from the SDK. This will not invalidate any existing IYubiKeyDevice instances, however you will not receive any additional events regarding that device. Any subsequent calls to YubiKeyDevice.FindAll, YubiKeyDevice.FindByTransport, or YubiKeyDeviceListener.Instance will restart the listening threads.
Suggested documentation updates describing the impact to the caller rather than what's it's doing under the covers.
| // Signal the listen thread that it's time to end. | ||
| _tokenSource.Cancel(); | ||
|
|
||
| // Shut down the listener handlers. |
There was a problem hiding this comment.
So really, we should be implementing this same pattern in the individual device type listeners. Because stopping this thread is not actually stopping the underlying devices from raising the PnP events. That needs to happen to for this story to really be complete.
| Update(); | ||
| try | ||
| { | ||
| await _semaphore.WaitAsync(CancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
We dropped a log message here. Knowing what started the update process is very useful to know.
Currently, YubiKeyDeviceListener only exposes a singleton instance with no way for a consumer to reset it once it is started. The constructor is private, so the consumer has no option to use a private instance.
This adds the ability for the consumer to reset the lazy-initialized singleton so that the listen thread doesn't stick around.
Also, even though this class implemented IDisposable, most things that need to be in the Dispose method weren't there, including a way to stop the thread.
The listen thread is now replaced by an asynchronous task that doesn't consume a system thread while it's not working.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # YESDK-1313
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test configuration:
Checklist:
dotnet formatto format my code