-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Dylan/resettable tcs #12453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dylan/resettable tcs #12453
Changes from all commits
95575c7
b86dd4c
d59f954
7907523
9f823f8
69ecbc8
f52af53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using BenchmarkDotNet.Attributes; | ||
| using Microsoft.AspNetCore.ConcurrencyLimiter.Tests; | ||
| using Microsoft.AspNetCore.Http; | ||
|
|
||
| namespace Microsoft.AspNetCore.ConcurrencyLimiter.Microbenchmarks | ||
| { | ||
| public class QueueRequestsOverwritten | ||
| { | ||
| private const int _numRejects = 5000; | ||
| private int _queueLength = 20; | ||
| private int _rejectionCount = 0; | ||
| private ManualResetEventSlim _mres = new ManualResetEventSlim(); | ||
|
|
||
| private ConcurrencyLimiterMiddleware _middlewareQueue; | ||
| private ConcurrencyLimiterMiddleware _middlewareStack; | ||
|
|
||
| [GlobalSetup] | ||
| public void GlobalSetup() | ||
| { | ||
| _middlewareQueue = TestUtils.CreateTestMiddleware_QueuePolicy( | ||
| maxConcurrentRequests: 1, | ||
| requestQueueLimit: 20, | ||
| next: WaitForever, | ||
| onRejected: IncrementRejections); | ||
|
|
||
| _middlewareStack = TestUtils.CreateTestMiddleware_StackPolicy( | ||
| maxConcurrentRequests: 1, | ||
| requestQueueLimit: 20, | ||
| next: WaitForever, | ||
| onRejected: IncrementRejections); | ||
| } | ||
|
|
||
| [IterationSetup] | ||
| public void Setup() | ||
| { | ||
| _rejectionCount = 0; | ||
| _mres.Reset(); | ||
| } | ||
|
|
||
| private async Task IncrementRejections(HttpContext context) | ||
| { | ||
| if (Interlocked.Increment(ref _rejectionCount) == _numRejects) | ||
| { | ||
| _mres.Set(); | ||
| } | ||
|
|
||
| await Task.Yield(); | ||
| } | ||
|
|
||
| private async Task WaitForever(HttpContext context) | ||
| { | ||
| await Task.Delay(int.MaxValue); | ||
| } | ||
|
|
||
| [Benchmark(OperationsPerInvoke = _numRejects)] | ||
| public void Baseline() | ||
| { | ||
| var toSend = _queueLength + _numRejects + 1; | ||
| for (int i = 0; i < toSend; i++) | ||
| { | ||
| _ = IncrementRejections(new DefaultHttpContext()); | ||
| } | ||
|
|
||
| _mres.Wait(); | ||
| } | ||
|
|
||
| [Benchmark(OperationsPerInvoke = _numRejects)] | ||
| public void RejectingRapidly_QueuePolicy() | ||
| { | ||
| var toSend = _queueLength + _numRejects + 1; | ||
| for (int i = 0; i < toSend; i++) | ||
| { | ||
| _ = _middlewareQueue.Invoke(new DefaultHttpContext()); | ||
| } | ||
|
|
||
| _mres.Wait(); | ||
| } | ||
|
|
||
| [Benchmark(OperationsPerInvoke = _numRejects)] | ||
| public void RejectingRapidly_StackPolicy() | ||
| { | ||
| var toSend = _queueLength + _numRejects + 1; | ||
| for (int i = 0; i < toSend; i++) | ||
| { | ||
| _ = _middlewareStack.Invoke(new DefaultHttpContext()); | ||
| } | ||
|
|
||
| _mres.Wait(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ public ConcurrencyLimiterOptions() { } | |
| public partial interface IQueuePolicy | ||
| { | ||
| void OnExit(); | ||
| System.Threading.Tasks.Task<bool> TryEnterAsync(); | ||
| System.Threading.Tasks.ValueTask<bool> TryEnterAsync(); | ||
| } | ||
| public partial class QueuePolicyOptions | ||
| { | ||
|
|
@@ -37,7 +37,7 @@ namespace Microsoft.Extensions.DependencyInjection | |
| { | ||
| public static partial class QueuePolicyServiceCollectionExtensions | ||
| { | ||
| public static Microsoft.Extensions.DependencyInjection.IServiceCollection AddFIFOQueue(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.AspNetCore.ConcurrencyLimiter.QueuePolicyOptions> configure) { throw null; } | ||
| public static Microsoft.Extensions.DependencyInjection.IServiceCollection AddLIFOQueue(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.AspNetCore.ConcurrencyLimiter.QueuePolicyOptions> configure) { throw null; } | ||
| public static Microsoft.Extensions.DependencyInjection.IServiceCollection AddQueuePolicy(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.AspNetCore.ConcurrencyLimiter.QueuePolicyOptions> configure) { throw null; } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be next to other stuff unrelated to to the ConcurrencyLimiter like collection.AddMvc(). I think this will make it confusing to most developers what this is adding a QueuePolicy to. Maybe this should be AddConcurrencyLimiterQueuePolicy() or AddConcurrencyLimiterStackPolicy(). I don't want to hold up the PR on this though. We can have a bigger discussion over naming and do a follow-up PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow up PR sounds good |
||
| public static Microsoft.Extensions.DependencyInjection.IServiceCollection AddStackPolicy(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.AspNetCore.ConcurrencyLimiter.QueuePolicyOptions> configure) { throw null; } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,19 +48,23 @@ public async Task Invoke(HttpContext context) | |
| { | ||
| var waitInQueueTask = _queuePolicy.TryEnterAsync(); | ||
|
|
||
| // Make sure we only ever call GetResult once on the TryEnterAsync ValueTask b/c it resets. | ||
| bool result; | ||
|
|
||
| if (waitInQueueTask.IsCompleted) | ||
| { | ||
| ConcurrencyLimiterEventSource.Log.QueueSkipped(); | ||
| result = waitInQueueTask.Result; | ||
| } | ||
| else | ||
| { | ||
| using (ConcurrencyLimiterEventSource.Log.QueueTimer()) | ||
| { | ||
| await waitInQueueTask; | ||
| result = await waitInQueueTask; | ||
| } | ||
| } | ||
|
|
||
| if (waitInQueueTask.Result) | ||
| if (result) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would any unit tests now fail if you were to check
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bit confused here. I thought the point of this change was to avoid calling GetResult twice. Wouldn't checking waitInQueueTask.Result here run into the same issue? |
||
| { | ||
| try | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.