Async support for data picker data source

nzdev commented 1 year ago

Description

Async support for data picker data source.

Types of changes

[ ] Documentation change
[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x] My code follows the coding style of this project.
[ ] My changes generate no new warnings.
[x] My change requires a change to the documentation.
[ ] I have updated the corresponding documentation.
[x] I have read the CONTRIBUTING and CODE_OF_CONDUCT documents.

leekelleher commented 1 year ago

Thank you for the PR @nzdev, much appreciated.

I'll get around to merging it in, but I do have a question - to be clear, not as a criticism of this PR - more to educate myself... "Why async all the things?"

I've been doing web dev a long time, and understand async more from a client-side/JS perspective - fire something off, don't wait for it, it'll come back when it's ready, then deal with it, non-blocking, etc. All good, totally get that.

But from a server-side/.NET perspective, I can't seem to get my head around it - I've asked many devs over the years and never really been satisfied with their answers.

Let's take the example of this PR, marking the API endpoints as async, what does that do? From the client-side, a HTTP request is made to the API endpoint, then it waits... but the API endpoint still has to do whatever it needs to do to get the data, so the HTTP response is going to wait for that, right? So where does the async bit come into play?

I'm probably after one concrete example of showing what .NET does (and why it's better) and I'll hopefully be sold on it.

In the case of Data Picker (or even Data List), what would async enable that regular sync (non-async) does not?

//cc @DanDiplo - noticed you gave a 👍, feel free to chime in. (Like I say, more an educational moment for me)

nzdev commented 1 year ago

From a server side perspective, these are the big reasons.

Modern c# APIs that involve communication, e.g HttpClient, databases, message queues, tcp, IO, etc have support for async and some don't have a synchronous API at all (HttpClient).
Because of the above, many c# libraries are designed for async.
The .net process has a limited number of threads in the thread pool. For async calls, when a call is awaited it's possible to return the current thread to the thread pool so it can do other work while waiting for a result. This improves scalability as the CPU is not stuck waiting.
The API for async is very easy to work with. So much so that it's been adopted by other languages like typescript and JavaScript.
Async in code is viral and needs to flow from the controller action all the way down to where you want to await. Not doing this makes api's difficult to use with async code.
Calling async code from a sync context has issues if not done right.
It's possible to provide a synchronous implemention of a method that returns a task and avoid the overhead involved with async by omitting the async keyword and using Task.Result.

DanDiplo commented 1 year ago

@leekelleher I won't repeat what @nzdev says, as he gets it spot on, but from the point of a consumer then a lot of modern API are async only. More of less anything that is REST related utilises HttpClient, which is async only. I've had a few use cases were I wanted to populate a DataList from a REST call and the APIs I'm calling are all async. As you know, if you want to call an async method correctly (and await) you need to do it from a method marked as async. This is why it's needed.

I'd also say that all of ASP.NET Core is async first:

"ASP.NET Core apps should be designed to process many requests simultaneously. Asynchronous APIs allow a small pool of threads to handle thousands of concurrent requests by not waiting on blocking calls. Rather than waiting on a long-running synchronous task to complete, the thread can work on another request.

A common performance problem in ASP.NET Core apps is blocking calls that could be asynchronous. Many synchronous blocking calls lead to Thread Pool starvation and degraded response times."

https://learn.microsoft.com/en-us/aspnet/core/fundamentals/best-practices?view=aspnetcore-7.0

So if I'm populating a DataList from (say) a remote server that is slow, that thread can be freed up and used for something else whilst it is waiting for the server to respond. That, I guess, is why we need an async version.

leekelleher commented 1 year ago

@nzdev @DanDiplo Thank you both, I appreciate the explanations. 🙏 @nzdev's point 3 makes most sense to me, e.g. avoiding thread pool starvation. 💯

I'll get this merged in soon. Thanks again.

kows commented 1 month ago

might be bumping an old PR but would the same for IDataListSource be a wanted change?

leekelleher commented 1 month ago

might be bumping an old PR but would the same for IDataListSource be a wanted change?

@kows Potentially for upcoming Contentment v6 (for Umbraco v14+), but not for the current version, it'd be a massive breaking-change. Still, there'd be a lot of work involved, (to retro-fit it in), guess I'd need to decide whether to delay the v6 release or consider it for the next major (v7). 🤔

leekelleher / umbraco-contentment