[SignalR] Avoid blocking common InvokeAsync usage

[BrennanConroy]

Proposal to fix #41997

Pros:

• Similar to streaming where once you start streaming we no longer consider the hub method blocking
• Fairly low code
• User code works as expected, and since you need parallel invokes enabled, they should be aware of thready safety concerns with accessing state after getting a client result

Cons:

• IHubContext injected and used in Hubs for InvokeAsync can still block
  • Likely will need an analyzer for this, or come up with some way to detect this/prevent this
• Semaphore acquire/release is more complex now
• Still need to define MaximumParallelInvocationsPerClient > 1

[gfoidl]

Just an idea:

Must _parallelInvokes be a SemaphoreSlim or can other sync-types be used?
E.g. with a bounded-channel (of maxInvokeLimit) the reader / writer pair can be used for synchronization too. IIf "waiting" will succeed synchronous, then the advantage is that it's a ValueTask, thus saving the allocation for the Task.
(Of course, if it real async mostly, then the benefit is much lower).

PS: in the past I ran some benchmarks (in my projects) with such an approach, and the channels variant looked better than SemaphoreSlim (due Task -> ValueTask mostly).

[BrennanConroy]

Channels would completely break the concept I'm trying to implement here, so while it might be better from a Task alloc perspective, it is much worse from a behavior perspective 😞

If we decide to go a different direction, then I think the Channel is worth revisiting to see if we can replace SemaphoreSlim.

[gfoidl]

Maybe we talked past each other.

I meant something like this

using System.Threading.Channels;

ChannelBasedSemaphore semaphore = new(1);

ValueTask task0 = semaphore.WaitAsync();
ValueTask task1 = semaphore.WaitAsync();

Console.WriteLine(task0.IsCompleted);   // true
Console.WriteLine(task1.IsCompleted);   // false

semaphore.Release();

await Task.Yield();
Console.WriteLine(task1.IsCompleted);   // true

internal class ChannelBasedSemaphore
{
    private readonly Channel<int> _channel;

    public ChannelBasedSemaphore(int initialCount)
    {
        _channel = Channel.CreateBounded<int>(initialCount);

        for (int i = 0; i < initialCount; ++i)
        {
            _channel.Writer.TryWrite(42);    // any dummy-value will do
        }
    }

    public void Release() => _channel.Writer.TryWrite(42);

    public async ValueTask WaitAsync(CancellationToken cancellationToken = default)
    {
        _ = await _channel.Reader.ReadAsync(cancellationToken);
    }
}

as replacement for SemaphoreSlim (kind of drop-in replacement).
(Note: only hacked togehter, no perf-optimization, etc.).

[BrennanConroy]

Using EchoAll - WS scenario on aspnet-perf-win:
Before:
~1.2m RPS
After with SemaphoreSlim:
~1.1m RPS
After with Channel:
~1.4m RPS

Allocations are hard to judge from the counters, but looking at perf view we can see both variations of this PR added a state machine (since we can't avoid it today even with Channels, maybe we can dig into it in 8).

[davidfowl]

Keep the channel, wrap it in a struct or class the hides the channel type and mimics the semaphore with a comment that we're saving allocations 😄

[davidfowl]

Do we need a limit here?

[BrennanConroy]

Do we need a limit here?

With a limit do we throw when another InvokeAsync call is made?

[davidfowl]

Lets keep it as is, no limit.

[halter73]

We should be delaying shutdown if the callbacks are still running. Is that happening? I have a feeling that ChannelBasedSemaphore will need to implement IAsyncDisposable or have some way to wait for all capacity to return before moving on.

[halter73]

I like it!

I assume that hub methods don't block the receive loop like streaming and client-invoking methods cannot delay shutdown. I don't think this is the most critical thing since streaming already behaves this way, but we should probably file a follow up issue.

[BrennanConroy]

I assume that hub methods don't block the receive loop like streaming and client-invoking methods cannot delay shutdown. I don't think this is the most critical thing since streaming already behaves this way, but we should probably file a follow up issue.

Right, so today if you enable parallel invokes we don't block the receive loop (unless you're at max invokes + 1) and consequently do delay some of the shutdown logic. Tomorrow (after this change), you won't block the receive loop for non-parallel invokes unless you have a pending invoke. So it's a slight change, but we do part of the shutdown logic always regardless of the receive loop state. So this is mostly delaying calling OnDisconnectedAsync and un-referencing connection state.

[adityamandaleeka]

Approved for RC1, this fixes a potentially bad issue in a new-to-7 feature.

[BrennanConroy]

Had to rebase due to conflicts with another change in test files.
@dotnet/aspnet-admins please merge (auto-merge?) when build passes (assuming HTTP3 doesn't fail on this PR)