-
Notifications
You must be signed in to change notification settings - Fork 5k
Add async support to System.Lazy #27510
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
Comments
I agree lazy async support is useful, but I'm very hesitant to see it added into the same type. Doing so would allow existing code to be able to use the same type without, for example, changing the type of the field storing the lazy object, but it would still require code changes, e.g. to use the new constructor and the new method or property, at which point that benefit decreases. And adding it into the same type necessarily adds both sync-over-async (i.e. if you construct the Lazy with an async delegate but then use the task-returning property) and async-over-sync (i.e. if you construct the Lazy with a sync delegate but then use the task-returning property, as presumably you'd want to queue the invocation and/or wait for the value to be available in case it was long-running). The existing thread safety modes aren't necessarily as applicable, or at the very least would need to be rationalized. Cancellation likely also becomes relevant. If we want to add async lazy support, I think it should be added as a dedicated type, and design thought put into what exactly it should look like, potentially with inspiration drawn from Roslyn's and the scenarios it's supporting, issues faced, etc. |
I agree that it shouldn't be added to the existing It's fairly easy to make a sort of static class Program
{
static async Task Main(string[] args)
{
var lazyGreeting = new Lazy<Task<string>>(createGreeting);
string greeting = await lazyGreeting;
Console.WriteLine($"{greeting} world!");
}
static async Task<string> createGreeting()
{
await Task.Delay(400);
return "hello";
}
public static TaskAwaiter<T> GetAwaiter<T>(this Lazy<Task<T>> asyncTask){
return asyncTask.Value.GetAwaiter();
}
} The only limitation here is that the constructor call is rather ugly (double nested generic) and it really only supports the The reason I agree we should make a separate class is because a |
+1 for |
@GSPP can you turn this into a formal API proposal based on the feedback. |
Er ... I think LazyTask is a nice design part, but I think it would be polluting the base library with an idiom. That seems like a pretty long stretch to justify including in a BCL. Heh also ... Wow! Then every class in the library can have a Task version! What is a Lazy that consumes a Task?? (Seems to be in fact just a Task.) What is a Task that produces a Lazy?? Bizarre! It's pretty tough to wrap around the contract in a base Api --- it seems to pivot out into many unknowns. It also becomes something that might spawn threads right in the Api. 2.00 US cents. |
I'm not sure what a formal API proposal would entail. But here is my attempt: class AsyncLazy<T>
{
AsyncLazy(Func<Task<T>> valueFactory, LazyThreadSafetyMode mode);
bool IsValueCreated { get; }
Task<T> GetValueAsync();
bool TryGetValue(out T value); //non-blocking
string ToString(); //the same as with Lazy<T>
} I dropped the (legacy?) I dropped the constructors that do not take a factory deleagte. I assume these just use the default constructor. There are no async constructors so this does not apply. (In my opinion having these constructors in the first place was a mistake as well but it does not matter for this issue.) This means that there's just a single constructor. I think that's a good thing. In my opinion the order of arguments is bad because the lambda could be a fairly long string and should be last. But I kept it this way to keep it consistent with My case for having What to do with The factory delegate will be called under a lock as before to ensure There is no async-over-sync or sync-over-async here which was a point mentioned in this thread. I decided to not make I decided to not add anything specific to make initialization cancellable. User code can see to it that the initialization task is aborted somehow and completes quickly in that case. The task that What should happen if the factory delegate throws (in contrast to the task becoming faulted)? I propose we treat it the same way as if the initialization task had faulted. What should happen if the initialization task faults or becomes cancelled? We could just forward the exception into the task returned by An open question is whether this should use
That's a good idea. Somebody needs to go do that now :) I should say that I will not be able to take on this work. But I hope I made a useful contribution by initiating the discussion and writing up this proposal. Feedback is very welcome. |
To echo @sharwell's concern over on dotnet/corefx#36078, an |
@AArnott So you're saying Roslyn implementation is incorrect? The problem with vs-threading is that it's VS specific. We need implementation that works anywhere (VS Code, VS4Mac, arbitrary app hosting Roslyn). Perhaps it needs to be parameterized to some extent. That's fine. |
No. I'm just saying that Roslyn's implementation is not sufficiently generalized for corefx. It requires that Roslyn-esque threading rules be followed (e.g. absolutely no UI thread dependency).
Why do you say that? The entire vs-threading library is completely independent of VS and it will always be so. It targets .NET Standard as well, and we run tests on all operating systems on both .NET Framework and .NET Core. In fact it's already running in VS for Mac. |
I guess I was confused by |
@sharwell @jasonmalinowski Any reason why do we not use AsyncLazy from VS threading in Roslyn? |
Ya, that's a common ailment of the name, which of course changing at this point would be rather costly.
The only feature Roslyn's implementation has that vs-threading's doesn't AFAIK is in multi-cancellation support. That is, yours will actually cancel the value factory if all the clients that requested the value cancel their requests. That's as I understand it anyway. And I'm not opposed to filling that gap either. I can't remember why we haven't done so already aside from lack of anyone asking for it. |
I'm not sure if Roslyn's AsyncLazy specifically really states any threading policy: you could use it however you want, but synchronously waiting on tasks on a UI thread will deadlock just as badly as waiting on a regular TPL Task will. 😄 The only special case really is ours also supports a GetValue which will do a synchronous wait if there's already an true async path running. (But that's something we need in specialized cases.)
Bingo: this is the one big reason we have ours, and why it gets so tricky! |
Would you switch to the one in vs-threading if we added that feature?
Well, I can't use the Roslyn implementation and follow JTF rules, thereby avoiding those deadlocks. Even in Roslyn, synchronously blocking the UI thread is sometimes desirable. VS and other GUI apps have their reasons too. And while Roslyn has very meticulously held any UI thread dependencies from async tasks in such cases at bay, in general that's a very hard problem to solve (perhaps impossible with backward compat if you don't control everything). That's where a JTF-aware AsyncLazy implementation because crucial. |
@AArnott taking into account the synchronization context and current task scheduler is a good point. Could we not just call the factory delegate under a null context? That's deterministic and likely what people want. It does not make sense to access the UI in a lazy initialization setting. Normally, initialization is about calling some service or computing something expensive. I'm a bit confused about what hazards you are seeing. Are you merely concerned about the initialization tasks accessing the UI or is it something else. |
Deadlock prevention in asynchronous code historically required global reasoning, which is all but impossible for large applications. Then when applications start refactoring code to use asynchronous operations where synchronous operations were used previously, the prior global reasoning effort is invalidated. @AArnott helped develop a library and set of rules that allow users to migrate applications from synchronous to asynchronous while only relying on local reasoning. The async lazy implementation from Roslyn does not adhere to the rules, so it cannot be used without falling back to global reasoning. If we want to create a generally usable
I believe the second would establish a very bad precedent. |
@sharwell I like your way of describing "local reasoning" vs. "global reasoning".
This would not be sufficient. Because even if an async method called
No, for two reasons:
Consider a value factory that needs the UI thread to complete. It is invoked through |
Hard to say: there's certain policy decisions Roslyn makes in our implementation (namely what to do in certain async + sync cases) which may or may not be appropriate in all cases. Also for example our AsyncLazy supports a non-caching mode because we have higher caching layers which is a crazy advanced use case. |
@AArnott @sharwell I don't understand all the details, but it seems to me there are essentially two scenarios:
Is this a correct way of thinking about this? |
I think that's close, but a bit too simple. It's not always clear whether a value factory does or does not have thread affinity. By default, any async method becomes affinitized to the main thread if that's where it started (because of the captured context). So I'd draw the line more like:
|
Yes, these rules are basically that I had in mind. These rules are easily followed in pure server code, perhaps less easy in a UI app. It helps if you layer your code in a way that separates any UI logic to a higher layer and purely computational/IO logic to lower layer. Then you only need to worry about callbacks passed in from the higher layer. This is how Roslyn is layered, but I think it's generally good practice rather than Roslyn specific design. I think it would make sense to provide AsyncLazy that requires following these rules in CoreFX, while another AsyncLazy implementation in a UI oriented library like MS.VS.Threading. Various analyzers accompanying these implementations would help enforcing the rules as much as possible. |
@AArnott I believe I found a better set of rules for a type that could be used in many scenarios:
These rules are not without limitations, but they force the type into a category where it can be used with or without JTF. |
This helped be understand the issue. In what way is the problem unique to async lazy? Should the normal Is it ever good design to have a lazy initializer body (async or not) access the UI? I have never seen that. Maybe I just haven't come across such code or such a pattern.
This does not work for many scenarios. AsyncLazy must be able to provide at-most-once execution. One example is side-effecting operations. Another example is avoiding the cache stampeding effect where many threads suddenly try to create the same expensive cache value. |
This is not strictly true, since the asynchronous initializer method can implement this protection itself when necessary. The big advantage to this is the asynchronous initializer can be written in a manner that correctly considers environmental requirements, including but not limited to JTF. |
This is the bulk of an "async lazy" implementation. If all of that is left to the delegate to implement, I don't know what the point of the type is. |
@sharwell I agree that if the 2 restrictions you prescribed were followed, a corefx AsyncLazy that wasn't JTF aware wouldn't be problematic (aside from the confusion it would introduce as the 3rd copy of AsyncLazy in some customer circles). But I agree with @stephentoub. Doing it right is very tricky. And discovering your mistake is unlikely before you've shipped your (possibly numerous) bugs. We need a type that customers will tend to pick that tends to do things right by default. Right now, the most public type folks can choose from is vs-threading's
@GSPP it's not typically that the value factory accesses UI elements. It's that it may access data structures that are either not thread-safe or are thread affinitized, and thus can only be accessed from the UI thread. And whether it's reading/mutating such data structures, or raising events to such data structures (e.g. the model is updating the view-model), it tends to be a real requirement fairly regularly in my experience.
@tmat No doubt Roslyn's design has some good "best practices" implemented in this area, but IMO it's a very high bar, and an unforgiving one. For folks in large apps and who want to progressively migrate their synchronous behaviors to async ones, I daresay it's nearly impossible. Roslyn had the benefit of being able to start from scratch for an enormous feature and could get it "right" from day 1. And the feature has enough weight behind it to influence partner teams to adapt to its threading requirements. When any of those conditions aren't true, folks need an option that allows them to progressively and flexibly migrate them from a sync to an async world, and that's where JTF and JTF-aware primitives become invaluable. |
I certainly agree when we're talking about large UI apps. However, although .NET Core is now adding support for UI apps, server side code (e.g. microservices) has always been .NET Core's main domain and there is no need to worry about JTF there. Providing two types that are used for these different types of apps would be imo reasonable. If we can make it a single type that can be parameterized then even better.
I don't see what the 3rd copy is. I'm proposing a new UI unaware implementation in CoreFX and the existing UI aware implementation in VS Threading. Unless we can have a single one in CoreFX parameterized by UI awareness, that is. |
Can you elaborate on what you have in mind? The vs-threading
I was counting the existing implementations from Roslyn and vs-threading, and the potential corefx one. |
I don't know what'd the best design be for a CoreFX primitive. Probably the less dependencies the better. The Roslyn implementation is internal, we would remove it once we could use a CoreFX alternative. |
Or: public class LazyAsync : Lazy |
@TitaniumIT Using What you're proposing isn't really async either. And doesn't enable something beyond what I could do with just |
…ito.AsyncEx (end of the post) to point directly to https://github.com/StephenCleary/AsyncEx instead of the now-defunct CodePlex. Further, there's a proposal to add an AsyncLazy (or equivalent) to the language at dotnet/runtime#27510 - if you have any thoughts on this, it would be really great if you could add them to that issue. https://blog.stephencleary.com/2012/08/asynchronous-lazy-initialization.html#comment-a4c0a2a0-0e07-11eb-ad39-4f7fd2834151
As the maintainer of a very commonly-used
|
Any movement in making this part of the BCL? |
There is another public implementation of With ambitious name, but it seems that there are few places where this implementation is used now. |
Still waiting for this to be added to the BCL, nearly half a decade later... |
All I wish for .NET 9 is nothing new shiny but the top upvoted issues to be finally implemented. 🤞 (This exact issue is currently ranked 20th with the 68 upvotes.) |
Lazy
is a very useful class. With async code becoming more and more common we should makeLazy
async aware. Doing that requires the following changes:Task
-returning factory delegates.Task<T> GetValueAsync()
If the synchronous
T Value { get; }
property is used then simply block on the task.Is
ValueTask
appropriate to use here?The text was updated successfully, but these errors were encountered: