davidfowl / aspnetcorediagnosticscenarios Goto Github PK
View Code? Open in Web Editor NEWThis repository has examples of broken patterns in ASP.NET Core applications
This repository has examples of broken patterns in ASP.NET Core applications
In https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md under "Async Void" the example code snippet won't compile anyway
The Guidance.md
doc mentions using AsyncLazy<T>
. I know Roslyn defines this internally, and the vs-threading library offers one publicly. Which one (or a new one?) are you proposing customers should use?
I know that this Repo is dedicated to AspNet core but which TaskCreationOptions should i take for .net versions below 4.6?
In .Net 4.5, TaskCreationOptions.RunContinuationsAsynchronously does not exist.
Great repo David.
Any best practice about ConfigureAwai(false)
? When is good to use it and when is not?ù
Thanks
I think at minimum, this advice should be clarified in two ways: example improved to actually show how a deadlock or other adversity can happen (example just shows when it'll run on the same thread, not how one gets in trouble when that happens; I don't have one in C# of the top of my head but see footnote2); and highlight performance trade-offs when this flag is used, as it does come at a penalty(!).
In fact, I spent couple of days thinking about it, and the more I'm thinking about it, the more I'm starting to lean toward that it may actually be a counter-productive (ok, bad 😄, forgive me 😹 ) advice. Here's my rationale.
There are two parts to my argument:
TaskCreationOptions.RunContinuationsAsynchronously
, and using it indiscriminately bears that cost.So, first, onto the performance part.
When a task is run in-line - that is, non-asynchronous continuation - it's run immediately as a direct function call. The cost of invoking that function is simply building the stack call frame for the function and making the execution jump to the first instruction of that function. And that has to happen regardless of inline or async continuation, and so inline execution is pretty much done at zero net cost.
Asynchronous continuation, however, is more involved. Instead of making that direct call, it is placed into a work queue. Another pool thread gets notified that there's available work, which then dequeues the work and only then calls the continuation function. So this trip through the queuing is the extra cost that is incurred with the TaskCreationOptions.RunContinuationsAsynchronously
.
Below example is from a part of TPL code - not sure if that's the exact part that handles this or something similar for some particular variation, but I think that's illustrative:
The above makes it pretty clear: if no RunContinuationsAsynchronously
is demanded, it's a direct call to the continuing action when it does c()
. But if it does demand asynchronous continuation, it sends the action down the Task.Run
's tunnels which "do a few things" (in fact, I was under impression that TPL used lock-less work-stealing queues, but I do see use of lock
in scheduling code, which adds to penalty), and those "few things" add [often] unnecessary performance cost. "Often" as in if flag used indiscriminately whether it's needed to prevent deadlock or if there is no possibility of deadlock even without it.
As I understand it, most tasks that result in calling async functions or task-returning functions default to inline execution. E.g. if you call a task-function without await
and that function never results in an operation that requires some resource completion later (e.g. you call Stream.ReadAsync()
where data was already in the buffer and didn't require waiting for a pending read operation to complete), the function will complete and continue to run the remaining part of the caller on the same thread. That's efficient. If crossing every task boundary was done via pool roundtrip, it would be much less performant (e.g. every Stream.ReadAsync()
call result to be forced through the pool so that non-io completion doesn't run inline).2
So making all TaskCompletionSource
s be async continuations may help reduce unsuspecting deadlocks or w/e issues due to poorly written caller, I suspect that accomplishes so at a substantial performance cost. This is where I think the advice is not complete in its recommendation, and is misleading.
The real issue is not that continuation may run inline! The real issue is that caller and/or continuation are not written properly to handle such a case! (Ergo, wrong solution at the wrong place.) Which brings me to the second argument.
You can't control all TaskCompletionSource
s, or rather that all TaskCompletionSource
s in a code path will use TaskCreationOptions.RunContinuationsAsynchronously
configuration option. It's not just a matter of changing it all in one's code! All 3rd party libraries, including .NET Core, would have to all be configured the same with that option. If one instance of TaskCompletionSource
in a library one doesn't control doesn't make use of this option, and the scenario is where it can cause the problem, then it can cause the problem b/c it may run inline. Thus, if one is diligent to always set TaskCreationOptions.RunContinuationsAsynchronously
on ALL their TaskCompletionSource
s in their code, it seems they're still screwed in case continuation is called from 3rd party library's TaskCompletionSource
that didn't set it. Back to SquareOne™!!! Except now you don't even suspect that that's an issue, because you've been diligent to have it run through the pool!
The problem with this solution, it seems to me, is that it requires the callee (i.e. code issuing TaskCompletionSource
instance) to do something special to play well with a caller that it doesn't know. It's much safer, though, when the caller and/or continuation makes itself safe and hardened against potential inline execution of continuation, and takes that burden upon itself, IMHO.
I suspect these are at least part of the reason TPL defaults to inline - to use more performant operation so as not to penalize by default, and because such issue is better resolved at the consumer rather than the producer end. (And doing so is even easier than the C++ example in the footnotes: with a simple await Task.Yield();
).
I may very well have missed something, or failed to consider, so very much welcome your thoughts on the above.
1 I mentioned earlier that I don't have a good C# example with which to improve, but I do have C++. One codebase I'm currently working on actually has in-house written C++ asynchronous library modeled, to a degree, on .NET's TPL. It has a notion similar to a promise, which, when resolved, will run all lambdas that were added to be notified of the resolution. It doesn't even have a concept of running such flag to call such functions via a thread pool. Such burden is placed on consumer of API to make such determination, and handle accordingly. And so example when such thing is done to avoid deadlock/exception is when caller is holding a non-recursive (non-reentrant) lock on std::mutex
while calling an asynchronous function. Since that function may or may not complete while holding a lock, the handler the explicitly moves further processing via a pool. But other code, when not in danger of inline mutex, would continue to run biz logic potentially inline since in those cases it's safe to do so, and is faster than always queueing to pool. So:
Promise A::DoLockedAsync(); // Must hold lock when calling.
Promise A::DoAsync(); // no need to hold lock to call.
void B::DoIt()
{
std::lock<std::mutex> l(_mutex);
A::DoLockedAsync().wait([](auto result) {
// This may run inline and we're holding a lock, so queue to pool.
ThreadPool.enqueue([result](){
std::lock_guard<std::mutex> g(_mutex);
ConsumeLocked(result) });
});
l.unlock();
A::DoAsync().wait([](auto result) {
// This may run inline and don't care if so.
Consume(result) });
});
}
... but I don't recall C# having non-recursive locks, and so, as I mentioned, can't seem to come up with another example.
2 This problem, by the way, is not native to .NET's TPL. Most JavaScript libraries that issue promises have it where attaching a handler to an already resolved promise will fire that handler inline, rather than via its message pump. AFAIK, these libraries do not attempt to avoid firing inline (by use of setTimeout(0, c)
, for example) and leave it up to the consumer of the API to handle so appropriately.
Arrived to this great repository after watching your great talk on scalability on youtube.
The situation here:
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#avoid-using-taskrun-for-long-running-work-that-blocks-the-thread
is relevant for us (we have ~10 such workers which keep synchronizing some database tables, each worker for a different table, into memory cache so that when http requests come we can handle them quickly using the data from the memory cache instead of querying the database) however we run some async calls which don't have alternative sync calls from that background worker and the example shown is sync so it is unclear how to correctly start async code in that new background thread without going back to using .Wait or .Result?
Maybe in such cases we need to use Task.Factory.StartNew with TaskCreationOptions.LongRunning which allows to smoothly run async task? and if so can you please provide an example of how to correctly do it because I saw that you wrote that its error prone if to pass wrong parameters
Thanks!
We often see code like that
public async Task<IActionResult> Get()
{
await Task.Delay(1000);
return View();
}
Is it a good practice to add the CancellationToken parameter to actions when it goes async and pass this parameter to async methods ?
public async Task<IActionResult> Get(int id, CancellationToken cancellationToken)
{
await Task.Delay(1000, cancellationToken);
return View();
}
In the AsyncGuidance document, there's a section that states that async/await
is preferred over directly returning Task
.
In his talk Correcting Common Async/Await Mistakes in .NET, Brandon Minnick advises to directly return the Task
over async/await
. (The specific bit is around 24:27.)
Since this is contradictory advice given by two MSFT employees, I feel like you guys should align this somehow, or clarify this use-case a bit more. :-)
IT might be less bad, but good???
There are legacy classes that use events and that need to use new async calls in their handlers. What is the suggested way of running async calls synchronously there?
Anyone aware of a Roslyn-based analyzer for the guidance “Prefer async/await over directly returning Task”?
/cc @p-schuler
In this situation, does Task.Run
is useful at all?
Knowing that FireAndForgetAsync
is not too important (it might raise exception, but we don't really care)
public async Task<IActionResult> DoSomething()
{
await repo.SaveDataAsync();
_ = Task.Run(FireAndForgetAsync);
}
public static async Task FireAndForgetAsync()
{
await repo.SaveOtherDataAsync();
}
Or
public async Task<IActionResult> DoSomething()
{
await repo.SaveDataAsync();
_ = FireAndForgetAsync();
}
public static async Task FireAndForgetAsync()
{
await repo.SaveOtherDataAsync();
}
You seems to use Task.Run
everywhere in examples, but is it really needed?
Could be possible to include the links in Guidance.md file into the README.md file so when visiting the repository navigation to each section becomes easier?
David, I figure you are a busy guy.. but I've noticed a discrepancy between .NET documentation and figured I'd get the best answer asking.
When it comes to the IHttpClientFactory.CreateClient
is it best to utilize using
given that it is an IDisposable? I know if I used typed clients I get disposal through DI.. Is it okay to inject clients into singletons, given that clients are supposed to be short lived?
Further.. should someone always using
an IDisposable
OR properly dispose via .Dispose()
in the event of a short lived asset?
Am I correct in understanding that "typically" IDispoable
s are meant for releasing unmanaged resources that might not be correctly disposed by GC... ?
or does GC call .Dispose()
?
I feel that a lot of disposal magic happens in DI, and so when working strictly inside a method that has some disposable assets I find myself getting a bit confused on what to using
Example of my typical flow (using httpclient as an example)
public async Task<Something> DoAsync()
{
using var client = _factory.CreateClient("MaybeName");
using var requestMessage = new HttpRequestMessage(....);
using var response = await client.SendAsync(requestMessage);
DoSomethingWithResponse...;
return Something;
}
Any guidance is appreciated
Avoid using Task.Result and Task.Wait:
ASP.NET Core Blazor does appear to use a synchronization context, being the RendererSynchronizationContext](https://source.dot.net/#Microsoft.AspNetCore.Components/Rendering/RendererSynchronizationContext.cs,11).
Hi David, example with AsyncLazy doesn't compile as AsyncLazy is not awaitable.
To fix that
var person = await _cache.GetOrAdd(id, (key) => new AsyncLazy<Person>(() => db.People.FindAsync(key)));
should be replaced with
var person = await _cache.GetOrAdd(id, (key) => new AsyncLazy<Person>(() => db.People.FindAsync(key))).Value;
There's a note regarding avoidance of TaskCreationOptions.LongRunning
:
💡 NOTE:
Task.Factory.StartNew
has an optionTaskCreationOptions.LongRunning
that under the covers creates a new thread and returns a Task that represents the execution. Using this properly requires several non-obvious parameters to be passed in to get the right behavior on all platforms.
The code sample then proceeds to use:
var thread = new Thread(ProcessQueue)
{
// This is important as it allows the process to exit while this thread is running
IsBackground = true
};
thread.Start();
The bit I'm missing is how we go from "API is awkward" to "look, you're better off with Thread.Start"; is it for reasons of cumbersome syntax or is it because it's outright better and/or TaskCreationOptions.LongRunning
should all-but be deprecated that we go to the lengths of actually creating a thread explicitly? I propose to use the answer to either
a) make a PR to switch the example to use Task.Factory.StartNew
, or
b) clarify the note to explicitly indicate that Task.Factory.StartNew
should be avoided, and (Thread {IsBackground = true}).Start()
is the recommended alternative
For instance in Serilog.Sinks.Async
, we create the thread using TaskCreationOptions.LongRunning
Not sure what a better header text would be, but
"❌ BAD This example uses Task.Result to get the connection in the constructor. This could lead to thread-pool starvation and deadlocks." is copy pasted in this section:
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#windowsidentityrunimpersonated
from this section:
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#constructors
and the context doesn't make sense.
For something like the following:
public class QueueProcessor { private readonly BlockingCollection _messageQueue = new BlockingCollection(); public void StartProcessing() { //Start background processing here. What's the best approach? } public void Enqueue(Message message) { _messageQueue.Add(message); } private async Task ProcessQueue() //notice the async Task here { foreach (var item in _messageQueue.GetConsumingEnumerable()) { await ProcessItem(item).ConfigureAwait(false); } } private async Task ProcessItem(Message message) { } }
What is the best approach of executing the long running work in the background? I have looked everywhere and I've found a lot of contradicting advice on this.
@davidfowl, Constructors section explains getting around by using static factory pattern.
But, what happens when I cannot call this static method to create an instance because the instance is created by container when IService
is dependency injected via constructor into other class?
Basically, what is the best practice for calling Async method in within constructor for these 2 different scenarios?
Task.Run(BackgroundOperationAsync);
?cc @pakrym
Reading the WindowsIdentity.RunImpersonated examples in the AsyncGuidance, there appear to be two errors:
context
parameter. However, RunImpersonated
does not take any delegate with parameters. It only takes Action
or Func<T>
(Func<Task>
or Func<Task<T>>
for RunImpersonatedAsync
), so why is this context
parameter in the examples?If we use async/await
in the A note about WebClient section of code then it will aligned with Prefer async/await over directly returning Task.
I'm not sure if it may add more confusion than it's worth, but should an exception for a long-running asynchronous process be included for clarity? By long-running async process I mean a process that runs using async/await so it doesn't block any thread pool threads but which executes for an extended period of time. To my understanding, this pattern is acceptable, though it comes with other complexities.
I've definitely encountered cases where the advice on long-running work was interpreted to also include this pattern. I've seen this pattern used with Task.Factory.StartNew
and TaskCreationOptions.LongRunning
in the false assumption it puts all the work on a separate thread. In reality, it makes a thread just for startup, and then the first await
puts the work back on the thread pool.
Example:
public class WorkerClass : IDisposable
{
private static readonly TimeSpan WorkInterval = TimeSpan.FromMinutes(5);
private readonly CancellationTokenSource _cts = new();
public void Start()
{
bool restoreFlow = false;
try
{
if (!ExecutionContext.IsFlowSuppressed)
{
ExecutionContext.SuppressFlow();
restoreFlow = true;
}
_ = DoWorkAsync();
}
finally
{
if (restoreFlow)
{
ExecutionContext.RestoreFlow();
}
}
}
public async Task DoWorkAsync()
{
while (!_cts.IsCancellationRequested)
{
await Task.Delay(WorkInterval, _cts.Token);
// Do async work here, assumption is the work item is not CPU intensive
}
}
public void Dispose()
{
_cts.Cancel();
_cts.Dispose();
}
}
I would be interested in some guidance how to handle events (e.g. WinForms) where you need to process something and put a value back to the event args (e.g. Cancel = true).
Hi, maybe this is worth mentioning. Basically, the FileOptions.Asynchronous
flag should be passed when creating a FileStream
(e.g. with File.Create
), otherwise the actual I/O operations are performed synchronously. More info.
I have to disagree on your statement:
Use of async void in ASP.NET Core applications is ALWAYS bad. Avoid it, never do it.
I know what the motivation is but the expression is wrong. async void has use case(s?) and should be used only if needed be. Such, maybe only, example is async event handler.
AspNetCoreDiagnosticScenarios/Scenarios/Services/RemoteConnection.cs
Lines 55 to 90 in 5be73c4
I believe it should be at least noted that if an exception occurs inside the factory function, the exception will be cached by the Task
and since everything is a singleton, the application won't be able to recover from the failed state without some restart (unless I'm missing something).
Obviously this may or may not be a problem, depends on the developer needs, but usually this isn't the intended behavior. Most commonly is like not adding an item to a cache if the initialization fails.
Hi, what's currently the recommended way to run async application initialization in asp.net core and .net core apps based on IGenericHost?
Wouldn't it be a nice idea to integrate async initialization into WebHost and GenericHost themselves? As an example we could use https://github.com/thomaslevesque/AspNetCore.AsyncInitialization/
What I am trying to achieve with that is:
I wouldn't agree on the statement that async void is evil. There are definitely exceptions where you're fine to use async void.
Moreover I'd really appreciate more details on how async void can cause process crashes when exceptions occur.
To summarize this first guideline, you should prefer async Task to async void. Async Task methods enable easier error-handling, composability and testability. The exception to this guideline is asynchronous event handlers, which must return void. This exception includes methods that are logically event handlers even if they’re not literally event handlers (for example, ICommand.Execute implementations).
When buffering you should still check Response.HasStarted before setting response headers (like the prior example. Buffers can be Flushed to start the response early.
There is no Contributors section in readme file .
As we know Contributions are what make the open-source community such an amazing place to learn, inspire, and create.
The Contributors section in a README.md file is important as it acknowledges and gives credit to those who have contributed to a project, fosters community and collaboration, adds transparency and accountability, and helps document the project's history for current and future maintainers. It also serves as a form of recognition, motivating contributors to continue their efforts.
Hello,
Very great list of best practices. It guides me and coworkers about multiple async await usage.
But there is 1 question where people disagree.
Regarding the following code
var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
await Task.WhenAll(task1, task2);
var result1 = task1.Result;
var result2 = task2.Result;
We agree that starting the 2 first tasks before waiting any result is good. But we disagree how to get the result.
By following best practices to get the value using await on each tasks is imo safer. But I do not have much arguments about it. I prefer the following version. It's more readable. But I miss some knowledge about performance and safety.
var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
var result1 = await task1;
var result2 = await task2;
What is your mind ? What are the best version regarding safety, performance, readability ?
Hi David,
Hope you are doing well!
Following your (awesome) async guidance, I'm having a strange behavior while testing a long-running message consumer with Task.Run vs Background Thread.
Code sample:
public async Task MainAsync(string[] args)
{
var backgroundThread = new Thread(Consume)
{
IsBackground = true
};
backgroundThread.Start(); //CPU - NOK
await Task.Run(Consume); //CPU - OK
}
private void Consume()
{
//while loop (sync) consuming messages from Kafka
}
Test conditions:
As we can see, pods running with background thread are struggling the CPU.
Do you have any thoughts why it happens?
Code: https://github.com/bmwhite20/cpu-workload
Best,
Bruno
Hi @davidfowl,
Trying to get rid of Task.Result in our code and follow your approach in AsyncGuidance.md#concurrentdictionarygetoradd by storing the Task instead of the actual value.
In our tests, it causes issue when the task fails (e.g. HTTP Exception) but the task obviously still gets added to the cache...
Any recommendations here?
Thanks,
Patrick
As discussed on twitter, I'm opening this issue about covering the usage of EF Query Async operations.
Is it wise to say that we should always use Async methods for query when it comes to EntityFramework Core (according to the ef core docs I would say it is but since you don't mention it) https://docs.microsoft.com/en-us/ef/core/querying/async
Do You Have This Rules as Sonarqube Rules?If you have rules,Can you send github repo?
I'm sorry, I don't have the answer, I'd just like to note that this is a common problem.
The note about WebClient contains some contradictory advice.
WebClient is considered a legacy .NET API at this point and has been completely superseded by HttpClient. No new code should be written with HttpClient.
Seems to suggest HttpClient should both:
Hi! Thanks for useful examples!
Let's say I've created a dedicated thread for some long running background work. Some part of the work involves connecting to some backend service using a ClientWebSocket, which has only async methods.
That way I have no options but .Wait() for the results, do I?
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.