dotnet / runtime

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

Consider providing async methods to, or an async alternative to, ManualResetEvent/ManualResetEventSlim #35962

Open john-h-k opened 4 years ago

john-h-k commented 4 years ago

We have SemaphoreSlim, with WaitAsync, but it is inherently different because it has counting semantics. Some form of MRE or MRESlim with a WaitOneAsync() would be useful. Searched for an issue but couldn't see one directly about this

Dotnet-GitSync-Bot commented 4 years ago

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

adams85 commented 4 years ago

@john-h-k FYI, plain WaitHandles (including ManualResetEvent) can be wrapped with an awaitable Task by means of ThreadPool.RegisterWaitForSingleObject as it's shown by Stephen Cleary in his excellent AsyncEx library. For the moment, I follow this approach when I need an awaitable AutoResetEvent/ManualResetEvent, however, I'm not sure it can be considered as a best practice. @jkotas Could you (or someone on your team) give us some pointers regarding this? Would an async-enabled MRE be a better choice?

As a side note, as for AREs I'm aware of another workaround: although it has different semantics, using a binary SemaphoreSlim we can simulate an awaitable ARE. This approach has a shortcoming though: when this simulated ARE is "set" via SemaphoreSlim.Release, it may throw a SemaphoreFullException if its counter has already reached the maximum, and these unwanted exceptions may hurt performance. Of course, the semaphore's counter could be checked before Release but not even this can completely eliminate the unwanted exceptions in heavily multi-threaded situations because check+release cannot be done atomically. However, a non-throwing TryRelease method would solve this problem and at least we'd have a viable solution for async AREs. @jkotas What are your thoughts on this? Would this idea of SemaphoreSlim.TryRelease be worth an API proposal by chance?

jkotas commented 4 years ago

I follow approach when I need an awaitable AutoResetEvent/ManualResetEvent, however, I'm not sure it can be considered as a best practice.

It works if you really need the full power of AutoResetEvent/ManualResetEvent for your scenario.

If you just need to signal that a work got finished via a Task, TaskCompletionSource is the most lightweight way to do that. Note that #17393 added a non-generic version of TaskCompletionSource for .NET 5 so it won't be necessary to use a dummy generic result argument anymore.

Would this idea of SemaphoreSlim.TryRelease be worth an API proposal

This does not look like a great API to me.

adams85 commented 4 years ago

@jkotas Alright, thanks for the info.

tactical-drone commented 4 years ago

@john-h-k , @adams85 , @jkotas @stephentoub

I have been trying to find a .net framework ZERO GC pressure semaphore that plays nice with async/await, but could not find one.

The best option right now is asyncautoresetevent, but that uses TaskCompletionSource. There is no support for a IValueTaskSource mutex/semaphore in the framework that I can find.

Therefor, I have been trying to build my own. My first attempt was based on trying to use the supplied ManualResetValueTaskSourceCore. But that leads to dark places.

My second iteration is a "native" semaphore instead. It depends on Spinlock. It is hot of the press and not fully tested. In fact I don't know how to test it. And I still need to add ExecutionContext logic to it. You could probably get rid of the dependence on Spinlock by using Interlocked.CompareExchange instead. I required Spinlock to implement auto scaling. So far it seems to be working well.

But it would be cool if seasoned .net veterans can look at this and tell me if it is a bad idea or not. Maybe help test. Maybe use it as an Idea for your next superior synchronization primitives.

Here is the source: https://gitlab.com/unimatrix-one/zero/-/blob/net5/zero.core/patterns/semaphore/core/IoZeroSemaphore.cs

kiminuo commented 3 years ago

Will this make .NET 6?

stephentoub commented 3 years ago

Will this make .NET 6?

Unlikely. For starters, it needs a concrete API proposal.

tactical-drone commented 3 years ago

@stephentoub I was wondering if you had a look at the code? Does it make sense?

kiminuo commented 3 years ago

Will this make .NET 6?

Unlikely. For starters, it needs a concrete API proposal.

😢

jez9999 commented 1 year ago

I'm a bit surprised at the lack of a WaitAsync in ManualResetEventSlim, too. Does anyone know whether just using Wait would have significant performance penalties in comparison?

I'm almost tempted to hack a SemaphoreSlim to act like a MRE by setting it to 0 and then releasing a very large number to mimic the "set" of a MRE (obviously assuming the number of threads waiting on the SemaphoreSlim is less than the very large number).

stephentoub commented 1 year ago

What's your use case? Why not just use a TaskCompletionSource? You need reset in a way that can't be accomplished by just replacing the instance?

jez9999 commented 1 year ago

Sorry, what do you mean replacing the instance?

stephentoub commented 1 year ago

if you need to reset back from set to unset, creating a new TaskCompletionSource and swapping it back in.

jez9999 commented 1 year ago

I guess that would work. Is that a complete replacement for ManualResetEventSlim then? Or are these some things the latter can do that a TCS can't?

stephentoub commented 1 year ago

Is there something about it that doesn't work for you?

jez9999 commented 1 year ago

I suppose where it's different is if some code holds a reference to the old TCS. The old TCS gets set, then replaced with a new one, but the code is still referencing the old one, and so if it awaits again it won't block. If it had been a MRE, resetting it would cause the code to block again even if it held onto the reference. In other words, for the "replace the instance" to work, the code has to always actively get the TCS instance from the right place when it waits for the signal.

jez9999 commented 1 year ago

@stephentoub Also, what if one needs to block on a MRE in both sync and async code? With SemaphoreSlim there's Wait and WaitAsync but awaiting a TCS is going to require an async context, isn't it? How could you use that like an MRE that could block both sync code and async code (with an await)?

stephentoub commented 1 year ago

In other words, for the "replace the instance" to work, the code has to always actively get the TCS instance from the right place when it waits for the signal.

Yes. But it cuts both ways. It's not uncommon for code to expect such transitions are one-way only, i.e. that you never reset, and TaskCompletionSource allows someone to depend on that: they know that once they've observed it set, they can observe it any number of additional times and it'll never show as unset. It really comes down to your scenario what you need, but in general if you support resets you need to be really careful about race conditions that can result from a thread missing a signal.

My question, though, wasn't about the general case; it was about your specific needs. Is there something about this that doesn't work for you?

but awaiting a TCS is going to require an async context, isn't it?

tcs.Task.Wait();

jez9999 commented 1 year ago

I guess that would work then. So how come ManualResetEventSlim can't just do that internally to allow sync and async wait? :smile:

stephentoub commented 1 year ago

It could... it would make every ManualResetEventSlim heavier weight and every operation performed on it more expensive.

jez9999 commented 1 year ago

I say it's worth it, especially considering it'd give parity with SemaphoreSlim.

jez9999 commented 1 year ago

@stephentoub I'm trying to create a generalized manual reset event class that allows for both sync and async waiting; does this look right to you?

using System.Threading.Tasks;

namespace UtilsCommon.Lib;

public sealed class ManualResetEventAsyncable {
    private TaskCompletionSource _tcs;

    /// <summary>
    /// Initializes a new instance of the ManualResetEventAsyncable class with an initial state of nonsignaled.
    /// </summary>
    public ManualResetEventAsyncable() : this(false) { }

    /// <summary>
    /// Initializes a new instance of the ManualResetEventAsyncable class with a Boolean value indicating whether
    /// to set the initial state to signaled.
    /// </summary>
    /// <param name="initialState">
    /// If true, indicates that the initial state will be set to signaled.
    /// If false, indicates that the initial state will be set to nonsignaled.
    /// </param>
    public ManualResetEventAsyncable(bool initialState) {
        _tcs = new TaskCompletionSource();
        if (initialState) {
            _tcs.SetResult();
        }
    }

    /// <summary>
    /// Gets whether the event is set.
    /// </summary>
    public bool IsSet => _tcs.Task.Status == TaskStatus.RanToCompletion;

    /// <summary>
    /// Sets the state of the event to nonsignaled, which causes threads to block.
    /// </summary>
    public void Reset() {
        if (IsSet) {
            _tcs = new TaskCompletionSource();
        }
    }

    /// <summary>
    /// Sets the state of the event to signaled, which allows one or more threads waiting on the event to proceed.
    /// </summary>
    public void Set() {
        _tcs.TrySetResult();
    }

    /// <summary>
    /// Blocks the current thread until the current ManualResetEventAsyncable is set.
    /// </summary>
    public void WaitSync() {
        _tcs.Task.Wait();
    }

    /// <summary>
    /// Asynchronously waits for the current ManualResetEventAsyncable to be set.
    /// </summary>
    /// <returns>A task that will complete when the current ManualResetEventAsyncable has been set.</returns>
    public Task WaitAsync() {
        return _tcs.Task;
    }
}
stephentoub commented 1 year ago

Looks reasonable.

You might also read https://devblogs.microsoft.com/pfxteam/building-async-coordination-primitives-part-1-asyncmanualresetevent/.

Also several nuget packages provide an async MRE, e.g. https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.threading.asyncmanualresetevent?view=visualstudiosdk-2022

jez9999 commented 1 year ago

Heh, yeah I must confess I spotted your blog post a few hours after making my comment. That link seems to be Visual Studio (or is it C++??) specific. How come such classes are under Microsoft.VisualStudio when they're not VS-specific at all?

stephentoub commented 1 year ago

It's not specific to Visual Studio. It was just written by the team that works on Visual Studio and is used by Visual Studio.

It's also not specific to C++; that page is just defaulting to showing code samples in C++. You can change the language for the samples in the language drop down on the upper right.

jez9999 commented 1 year ago

Ah, I see. Fair enough.

By the way, did you ever implement a reader/writer lock with both sync and async lock methods? I've been creating something based on your blog post which seems to work but only has async lock methods:

using System;
using System.Collections.Generic;
using System.Threading.Tasks;

namespace UtilsCommon.Lib;

/// <summary>
/// Class that provides (optionally async-safe) read/write locking using internal task completion source.
/// Bear in mind that all code executed inside the using statement must finish before the next thread is able to
/// start executing it, so long-running code should be avoided inside the using statement if at all possible.
///
/// Example usage for async:
/// using (mutex.[Read/Write]LockAsync()) {
///     // ... code here which can use await calls and handle a shared resource one-thread-at-a-time ...
///     return[ result];
/// }
/// </summary>
public sealed class ReadWriteMutexAsyncable {
    #region Internal classes

    private sealed class Releaser : IDisposable {
        private readonly ReadWriteMutexAsyncable _toRelease;
        private readonly bool _isWriter;
        internal Releaser(ReadWriteMutexAsyncable toRelease, bool isWriter) {
            _toRelease = toRelease;
            _isWriter = isWriter;
        }
        public void Dispose() {
            if (_isWriter) { _toRelease.writerRelease(); }
            else { _toRelease.readerRelease(); }
        }
    }

    #endregion

    private readonly Task<IDisposable> _readerReleaser;
    private readonly Task<IDisposable> _writerReleaser;
    private readonly Queue<TaskCompletionSource<Releaser>> _waitingWriters = new();
    private TaskCompletionSource<Releaser> _waitingReader = new();
    private int _readersWaiting;
    private int _status;
    // ^ If -1, locked for a writer.  If 0, no locks acquired.  If positive, indicates number of readers with lock.

    /// <summary>
    /// Creates a new ReadWriteMutexAsyncable instance.
    /// </summary>
    public ReadWriteMutexAsyncable() {
        // Releaser instances that'll allow us to have the option of immediately returning completed tasks if
        // the lock was acquired immediately.
        _readerReleaser = Task.FromResult((IDisposable)new Releaser(this, false));
        _writerReleaser = Task.FromResult((IDisposable)new Releaser(this, true));
    }

    private void readerRelease() {
        TaskCompletionSource<Releaser>? toWake = null;

        lock (_waitingWriters) {
            _status--;
            if (_status == 0 && _waitingWriters.Count > 0) {
                _status = -1;
                toWake = _waitingWriters.Dequeue();
            }
        }

        toWake?.SetResult(new Releaser(this, true));
    }

    private void writerRelease() {
        TaskCompletionSource<Releaser>? toWake = null;
        bool toWakeIsWriter = false;

        lock (_waitingWriters) {
            if (_waitingWriters.Count > 0) {
                toWake = _waitingWriters.Dequeue();
                toWakeIsWriter = true;
            }
            else if (_readersWaiting > 0) {
                toWake = _waitingReader;
                _status = _readersWaiting;
                _readersWaiting = 0;
                _waitingReader = new TaskCompletionSource<Releaser>();
            }
            else {
                _status = 0;
            }
        }

        toWake?.SetResult(new Releaser(this, toWakeIsWriter));
    }

    /// <summary>
    /// Obtains a read lock asynchronously.
    /// </summary>
    /// <returns>An IDisposable that will release the obtained read lock upon being disposed.</returns>
    public Task<IDisposable> ReadLockAsync() {
        lock (_waitingWriters) {
            if (_status >= 0 && _waitingWriters.Count == 0) {
                _status++;
                return _readerReleaser;
            }
            else {
                _readersWaiting++;
                return _waitingReader.Task.ContinueWith(t => (IDisposable)t.Result);
            }
        }
    }

    /// <summary>
    /// Obtains a write lock asynchronously.
    /// </summary>
    /// <returns>An IDisposable that will release the obtained write lock upon being disposed.</returns>
    public Task<IDisposable> WriteLockAsync() {
        lock (_waitingWriters) {
            if (_status == 0) {
                _status = -1;
                return _writerReleaser;
            }
            else {
                var waiter = new TaskCompletionSource<Releaser>();
                _waitingWriters.Enqueue(waiter);
                return waiter.Task.ContinueWith(t => (IDisposable)t.Result);
            }
        }
    }
}

I started writing sync versions of the lock methods but presumably the blocking on Task.Wait() within the lock is going to cause deadlock:

    public IDisposable ReadLockSync() {
        lock (_waitingWriters) {
            if (_status >= 0 && _waitingWriters.Count == 0) {
                _status++;
                return _readerReleaser.Result;
            }
            else {
                _readersWaiting++;
                _waitingReader.Task.Wait();
                return _waitingReader.Task.Result;
            }
        }
    }

    public IDisposable WriteLockSync() {
        lock (_waitingWriters) {
            if (_status == 0) {
                _status = -1;
                return _writerReleaser.Result;
            }
            else {
                var waiter = new TaskCompletionSource<Releaser>();
                _waitingWriters.Enqueue(waiter);
                waiter.Task.Wait();
                return waiter.Task.Result;
            }
        }
    }

How would you implement the sync methods?

timcassell commented 2 months ago

FWIW, I implemented all the synchronization primitives supporting both async and sync in my promise library. Feel free to check the source code and tests (and concurrency tests) if you're having troubling implementing your own.

lostmsu commented 3 weeks ago

@jez9999 @stephentoub omg, how does it look reasonable? Don't give bad advise like that please. The code sample has no memory barriers and threads other than the one that called Reset may never see the new value of _tcs

jez9999 commented 3 weeks ago

@lostmsu Yeah, I just use AsyncEx now. Bit annoying that .NET doesn't have this stuff built in but that works well.