dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.56k stars 4.54k forks source link

[API Proposal]: Add `System.Threading.ConditionVariable` type #104179

Open timcassell opened 1 week ago

timcassell commented 1 week ago

Background and motivation

System.Threading.Lock type was added to replace Monitor.Enter/Exit (#34812). No replacements were added for Monitor.Wait/Pulse. ConditionVariable is the replacement for those, as well as it can be used locally (Monitor.Wait/Pulse can only be used globally).

It's also .Net's version of C++'s std::condition_variable.

API Proposal

namespace System.Threading;

public sealed class ConditionVariable
{
    public ConditionVariable(Lock @lock);
    public void Wait();
    public bool TryWait(TimeSpan timeout);
    public bool TryWait(int millisecondsTimeout);
    public void Notify();
    public void NotifyAll();
}

Notify naming matches C++, but it could also be Pulse to match Monitor instead, or even Signal to match WaitHandle.

API Usage

The pattern using Monitor:

private readonly Queue<T> _queue = new();
private readonly object _mutex = new();

public void PushItem(T item)
{
    lock (_mutex)
    {
        _queue.Enqueue(item);
        Monitor.Pulse(_mutex);
    }
}

public T GetItem()
{
    lock (_mutex)
    {
        while (!_queue.TryDequeue(out T item))
        {
            Monitor.Wait(_mutex);
        }
        return item;
    }
}

Converted to using the new types (with C# updated lock keyword):

private readonly Queue<T> _queue = new();
private readonly Lock _lock = new();
private readonly ConditionVariable _condVar = new(_lock);

public void PushItem(T item)
{
    lock (_lock)
    {
        _queue.Enqueue(item);
        _condVar.Notify();
    }
}

public T GetItem()
{
    lock (_lock)
    {
        while (!_queue.TryDequeue(out T item))
        {
            _condVar.Wait();
        }
        return item;
    }
}

More advanced usage that is more cumbersome to implement with Monitor:

private readonly Lock _lock = new();
private readonly ConditionVariable _condVar1 = new(_lock);
private readonly ConditionVariable _condVar2 = new(_lock);

public void WaitForEvent1()
{
    lock (_lock)
    {
        _condVar1.Wait();
    }
}

public void WaitForEvent2()
{
    lock (_lock)
    {
        _condVar2.Wait();
    }
}

public void SignalEvent1()
{
    lock (_lock)
    {
        _condVar1.Notify();
    }
}

public void SignalEvent2()
{
    lock (_lock)
    {
        _condVar2.Notify();
    }
}

Alternative Designs

Alternative 1 - A ConditionVariable may be associated with more than 1 Lock object, but not more than 1 at the same time. The Lock is passed as an argument to each function, and it's validated internally to be only used by 1 Lock at a time. In C++, using the same condition variable with more than 1 mutex at the same time has undefined behavior. .Net should throw.

namespace System.Threading;

public sealed class ConditionVariable
{
    public void Wait(Lock @lock);
    public bool TryWait(Lock @lock, TimeSpan timeout);
    public bool TryWait(Lock @lock, int millisecondsTimeout);
    public void Notify(Lock @lock);
    public void NotifyAll(Lock @lock);
}

Alternative 2 - Match std::condition_variable more closely.

namespace System.Threading;

public sealed class ConditionVariable
{
    public void Wait(Lock @lock);
    public bool TryWait(Lock @lock, TimeSpan timeout);
    public bool TryWait(Lock @lock, int millisecondsTimeout);
    public void Notify();
    public void NotifyAll();
}

This version does not take the Lock object as an argument for Notify(All). Even though C++ does it this way, I think it's less safe than passing in the Lock object (we can verify that the notification occurs for the correct Lock object).

Open Question

Monitor.Pulse(All) requires the lock to be held while calling it. std::condition_variable::notify_one has this in its notes:

The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock.

Should ConditionVariable.Notify(All) require the lock to be entered? (It might need it as an implementation detail, I'm not sure.)

Risks

None

dotnet-policy-service[bot] commented 1 week ago

Tagging subscribers to this area: @mangod9 See info in area-owners.md if you want to be subscribed.

jkotas commented 1 week ago

https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Condition.cs is an internal implementation of this used by native AOT.

Should lock be a constructor parameter instead of being passed into every instance method?

timcassell commented 1 week ago

Should lock be a constructor parameter instead of being passed into every instance method?

That's the alternative 1 design. I prefer passing it in to each method to be less restrictive so that multiple Lock instances can use a single ConditionVariable instance (not at the same time). This makes it possible to pool the objects and use them later without worrying about mapping the exact Lock object to it. I implemented an AsyncConditionVariable in my promise library that works this way.

timcassell commented 6 days ago

Upon further thought, I can't think of a reason to need to pool individual ConditionVariable objects. They would typically be used inside a data structure, and if object pooling is needed, the data structure itself can be pooled. So I guess it makes sense to only pass in the Lock as a constructor argument to reduce overhead and simplify the API.

Perhaps a hypothetical ValueConditionVariable struct could take a hypothetical ref ValueLock struct as an argument to each method (to reduce object overhead), and that would more closely match C++'s semantics, but it makes less sense for the ConditionVariable class. But I'm getting ahead of myself.

I updated the proposal.

mangod9 commented 3 days ago

@kouvel