Skip to content

Commit

Permalink
Make YubiKeyDeviceListener resettable
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jamiehankins
jamiehankins committed Aug 16, 2024
1 parent e1dbe19 commit 63eea0e
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 57 deletions.
3 changes: 3 additions & 0 deletions Yubico.Core/tests/Yubico/Core/Devices/Hid/HidTranslatorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,10 @@ public void TestSpecializedKeyboardSupportsModhexString(KeyboardLayout layout)
#if Windows
#pragma warning disable CA1825
[Theory]
#pragma warning disable CA1825 // Avoid zero-length array allocations
// The compiler mistakenly thinks that this somehow involves a zero-length array.
[MemberData(nameof(GetTestData))]
#pragma warning restore CA1825 // Avoid zero-length array allocations
public void GetChar_GivenHidCode_ReturnsCorrectChar(KeyboardLayout layout, (char, byte)[] testData)
{
var hid = HidCodeTranslator.GetInstance(layout);
Expand Down
138 changes: 81 additions & 57 deletions Yubico.YubiKey/src/Yubico/YubiKey/YubiKeyDeviceListener.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System.Linq;
using System.Threading;
using Microsoft.Extensions.Logging;
using System.Threading.Tasks;
using Yubico.Core.Devices;
using Yubico.Core.Devices.Hid;
using Yubico.Core.Devices.SmartCard;
Expand Down Expand Up @@ -44,82 +45,84 @@ public class YubiKeyDeviceListener : IDisposable
/// <summary>
/// An instance of a <see cref="YubiKeyDeviceListener"/>.
/// </summary>
public static YubiKeyDeviceListener Instance => _lazyInstance.Value;
public static YubiKeyDeviceListener Instance => _lazyInstance ??= new YubiKeyDeviceListener();

private static readonly Lazy<YubiKeyDeviceListener> _lazyInstance =
new Lazy<YubiKeyDeviceListener>(() => new YubiKeyDeviceListener());
/// <summary>
/// Disposes and closes the singleton instance of <see cref="YubiKeyDeviceListener"/>.
/// </summary>
public static void ResetInstance()
{
if (_lazyInstance != null)
{
_lazyInstance.Dispose();
_lazyInstance = null;
}
}

private static readonly ReaderWriterLockSlim RwLock = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
private static YubiKeyDeviceListener? _lazyInstance;

private readonly ILogger _log = Log.GetLogger<YubiKeyDeviceListener>();
private readonly ReaderWriterLockSlim _rwLock = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);

private readonly Logger _log = Log.GetLogger<YubiKeyDeviceListener>();
private readonly Dictionary<IYubiKeyDevice, bool> _internalCache = new Dictionary<IYubiKeyDevice, bool>();
private readonly HidDeviceListener _hidListener = HidDeviceListener.Create();
private readonly SmartCardDeviceListener _smartCardListener = SmartCardDeviceListener.Create();

private readonly Thread? _listenerThread;
private readonly bool _isListening;
private readonly SemaphoreSlim _semaphore = new SemaphoreSlim(1);
private readonly Task _listenTask;
private readonly CancellationTokenSource _tokenSource = new CancellationTokenSource();
private CancellationToken CancellationToken => _tokenSource.Token;

private bool _isListening;

private YubiKeyDeviceListener()
{
_listenerThread = new Thread(ListenForChanges) { IsBackground = true };
_isListening = true;
_log.LogInformation("Creating YubiKeyDeviceListener instance.");

SetupDeviceListeners();

_log.LogInformation("Performing initial cache population.");
Update();

_listenerThread.Start();
_listenTask = ListenForChanges();
}

internal List<IYubiKeyDevice> GetAll() => _internalCache.Keys.ToList();

private void ListenForChanges()
{
using var updateEvent = new ManualResetEvent(false);

_log.LogInformation("YubiKey device listener thread started. ThreadID is {ThreadID}.", Environment.CurrentManagedThreadId);

_smartCardListener.Arrived += (s, e) =>
{
_log.LogInformation("Arrival of smart card {SmartCard} is triggering update.", e.Device);
_ = updateEvent.Set();
};

_smartCardListener.Removed += (s, e) =>
{
_log.LogInformation("Removal of smart card {SmartCard} is triggering update.", e.Device);
_ = updateEvent.Set();
};

_hidListener.Arrived += (s, e) =>
{
_log.LogInformation("Arrival of HID {HidDevice} is triggering update.", e.Device);
_ = updateEvent.Set();
};
private void ListenerHandler(object? sender, EventArgs e) => _semaphore.Release();

_hidListener.Removed += (s, e) =>
{
_log.LogInformation("Removal of HID {HidDevice} is triggering update.", e.Device);
_ = updateEvent.Set();
};
private void SetupDeviceListeners()
{
_smartCardListener.Arrived += ListenerHandler;
_smartCardListener.Removed += ListenerHandler;
_hidListener.Arrived += ListenerHandler;
_hidListener.Removed += ListenerHandler;
}

private async Task ListenForChanges()
{
_isListening = true;
while (_isListening)
{
_ = updateEvent.WaitOne();
Thread.Sleep(200); // I really dislike sleeps, but here, it does seem like a good idea to give the
// system some time to quiet down in terms of PnP activity.
_ = updateEvent.Reset();
Update();
try
{
await _semaphore.WaitAsync(CancellationToken).ConfigureAwait(false);
// Give events a chance to coalesce.
await Task.Delay(200, CancellationToken).ConfigureAwait(false);
Update();
// Reset any outstanding events.
_ = await _semaphore.WaitAsync(0, CancellationToken).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
break;
}
}

// KeepAlive seems to be necessary here as the collector doesn't know that it shouldn't dispose the
// event until the very end of this function/thread.
GC.KeepAlive(updateEvent);
}

private void Update()
{
RwLock.EnterWriteLock();
_rwLock.EnterWriteLock();
_log.LogInformation("Entering write-lock.");

ResetCacheMarkers();
Expand Down Expand Up @@ -209,7 +212,7 @@ private void Update()
}

_log.LogInformation("Exiting write-lock.");
RwLock.ExitWriteLock();
_rwLock.ExitWriteLock();
}

private List<IDevice> GetDevices()
Expand Down Expand Up @@ -349,23 +352,46 @@ private static void ErrorHandler(Exception exception) =>
.GetLogger(typeof(YubiKeyDeviceListener).FullName!)
.LogWarning($"Exception caught: {exception}");

#region IDisposable Support

private bool _disposedValue;
private bool _isDisposed;

/// <summary>
/// Disposes the objects.
/// </summary>
/// <param name="disposing"></param>
protected virtual void Dispose(bool disposing)
{
if (!_disposedValue)
if (!_isDisposed)
{
if (disposing)
{
RwLock.Dispose();
// Signal the listen thread that it's time to end.
_tokenSource.Cancel();

// Shut down the listener handlers.
_hidListener.Arrived -= ListenerHandler;
_hidListener.Removed -= ListenerHandler;
if (_hidListener is IDisposable hidDisp)
{
hidDisp.Dispose();
}

_smartCardListener.Arrived -= ListenerHandler;
_smartCardListener.Removed -= ListenerHandler;
if (_smartCardListener is IDisposable scDisp)
{
scDisp.Dispose();
}

// Give the listen thread a moment (likely is already done).
_ = !_listenTask.Wait(100);
_listenTask.Dispose();

// Get rid of synchronization objects.
_rwLock.Dispose();
_semaphore.Dispose();
_tokenSource.Dispose();
}
_disposedValue = true;
_isDisposed = true;
}
}

Expand All @@ -385,7 +411,5 @@ public void Dispose()
Dispose(true);
GC.SuppressFinalize(this);
}

#endregion
}
}

0 comments on commit 63eea0e

Please sign in to comment.