Skip to content
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

Implement AsyncLazyInitializer #31060

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Nov 9, 2018

This is a supporting type for #30929, but can also support many other use cases where lock-free optimistic initialization is desirable but the initialization function is asynchronous.

@sharwell sharwell requested a review from a team as a code owner November 9, 2018 15:28
@sharwell
Copy link
Member Author

sharwell commented Nov 9, 2018

/cc @drewnoakes @stephentoub @AArnott

@AArnott
Copy link
Contributor

AArnott commented Nov 9, 2018

FWIW, we recently added an AsyncLazyInitializer to vs-threading. It takes a very different approach, and is JTF aware:

https://github.com/Microsoft/vs-threading/blob/master/src/Microsoft.VisualStudio.Threading/AsyncLazyInitializer.cs

@sharwell
Copy link
Member Author

sharwell commented Nov 9, 2018

FWIW, we recently added an AsyncLazyInitializer to vs-threading.

You should rename that type to avoid confusion with LazyInitializer. They address totally separate situations. microsoft/vs-threading#413

and is JTF aware

This type is JTF-agnostic.

@jasonmalinowski jasonmalinowski changed the base branch from dev16.0-preview2 to master November 16, 2018 19:40
@sharwell sharwell changed the title Implement AsyncLazyInitializer 🚧 Implement AsyncLazyInitializer Apr 17, 2019
Base automatically changed from master to main March 3, 2021 23:51
@sharwell
Copy link
Member Author

it's a bit odd since hte parameter name is actually 'valueFactory'. Perhaps 'valueFactory' returned null?

This was also taken directly from LazyInitializer:

https://github.com/dotnet/runtime/blob/35795c52d5cfe635152e4c39e037f92a2710c413/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx#L2713-L2715

This comment may not be particularly useful to people (like me) who didn't know what the other one was either...

The new hyperlinking support of Quick Info should help with clarity here. With that said, LazyInitializer''s documentation is "succinct".

In the "we can't deal with IDisposable", why use this class entirely? Is it actually any faster than just inlining the operation?

Overloads are provided that support automatically releasing unused values.

Why is this broken into a separate method vs just inlined?

Each helper is now used from two locations.

I must admit, this feels really screwy. I get it, but it seems like us lifting local variables in the lambda and such would offset some of the benefits of this...?

I'm not aware of an alternative. The API is still cleaner than trying to inline the functionality correctly.

Why not break into separate test methods?

Done.

I admit the test name predisposed me to think "firstInitializedValueTask" would be the one that is taken. The code is right, the naming was a bit offputting though.

Adding some comments to clarify the weirdness here.

@sharwell sharwell changed the title 🚧 Implement AsyncLazyInitializer Implement AsyncLazyInitializer May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants