I have a class from an assembly loaded at runtime that is instantiated using reflection (and therefore using a parameter-less constructor) across multiple threads (using Channels). Each thread instantiates an instance of the class and then calls Method()
with a bunch of arguments.
One of the arguments contains licence keys that the plugin might need in order to use its plugins. It should apply this key only once.
My current implementation uses double-check locking:
private static bool _IsLicenceApplied;
private readonly static object _IsLicenceAppliedInitializationLock = new object();
public void Method(Keys keys, object data)
{
if(!_IsLicenceApplied)
{
lock(_IsLicenceAppliedInitializationLock)
{
if(!_IsLicenceApplied)
{
Apply(keys);
_IsLicenceApplied = true;
}
}
}
//do the rest of the method using data
}
I've read through countless SO answers, blog posts and documentation that contain conflicting information and opinions about whether:
_IsLicenceApplied
should be volatile
_IsLicenceApplied
should be read and written to using the methods on the Volatile
classInterlocked
classThread.MemoryBarrier()
A lot of these talk about the Singleton pattern and replacing such code with Lazy<T>
but I need to pass the keys into the method and have the first execution deal with them. I don't think that's possible with Lazy<T>
. Perhaps there's another built-in class that makes this possible?
Edit: added _IsLicenceApplied = true;
after calling Apply()
.
Edit 2: Possible solution after reading source code of LazyInitializer (which I could use instead?):
private static bool _IsLicenceApplied;
private static object _IsLicenceAppliedInitializationLock = new object();
public void Method(Keys keys, object data)
{
if(!Volatile.Read(_IsLicenceApplied))
{
lock(_IsLicenceAppliedInitializationLock)
{
if(!Volatile.Read(_IsLicenceApplied))
{
Apply(keys);
Volatile.Write(ref _IsLicenceApplied, true);
}
}
}
//do the rest of the method using data
}
Your first implementation, without using the Volatile
, is incorrect. It allows the possibility of a compiler optimization that could cause a thread to continue before the completion of the Apply(keys)
invocation. This possibility is remote, mostly theoretical, but nevertheless this implementation would be disqualified as incorrect by any reviewer who considers absolute correctness to be a non-negotiable requirement.
The second implementation, with the use of Volatile
, is correct. Nevertheless it has the behavior of the LazyInitializer.EnsureInitialized
API, of which I am not a big fan. The questionable behavior emerges in case the Apply(keys)
fails repeatedly, in which case all waiting threads will retry the execution one by one, resulting potentially to accumulated long delays. My preference is to propagate the error of the failed execution to all threads that are currently waiting. This way no thread is going to wait for longer than the duration of a single invocation. Below is a SingleExecution
class with this behavior:
/// <summary>
/// Represents a synchronous operation that is invoked on demand only once, unless
/// it fails, in which case it is retried as many times as needed until it succeeds.
/// </summary>
/// <remarks>
/// In case of failure the error is propagated to the invoking thread,
/// as well as to all other threads that are currently waiting for the execution.
/// </remarks>
public class SingleExecution
{
private volatile Task _task;
public void EnsureInvoked<TArg>(Action<TArg> action, TArg arg)
{
ArgumentNullException.ThrowIfNull(action);
Task capturedTask = _task;
if (capturedTask is null)
{
Task newTask = new(() =>
{
try
{
action(arg);
_task = Task.CompletedTask;
}
catch
{
_task = null;
throw;
}
});
capturedTask = Interlocked
.CompareExchange(ref _task, newTask, null) ?? newTask;
if (ReferenceEquals(capturedTask, newTask))
newTask.RunSynchronously(TaskScheduler.Default);
}
capturedTask.GetAwaiter().GetResult();
}
}
This implementation is a modified version of the AsyncLazy
class that I have posted here.
The same principle regarding handling exceptions, albeit with a different implementation, has been employed in a LazyWithRetry
class that I've posted here.
Usage example:
private readonly SingleExecution _applyKeysExecution = new();
public void Method(Keys keys, object data)
{
_applyKeysExecution.EnsureInvoked(k => Apply(k), keys);
//...
}