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

Microsoft.Extensions.AsyncState readme suggestions #4611

Closed
KalleOlaviNiemitalo opened this issue Oct 24, 2023 · 5 comments · Fixed by #4764
Closed

Microsoft.Extensions.AsyncState readme suggestions #4611

KalleOlaviNiemitalo opened this issue Oct 24, 2023 · 5 comments · Fixed by #4764
Assignees
Labels
area-fundamentals documentation enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Oct 24, 2023

Suggestions for things that could be added to src/Libraries/Microsoft.Extensions.AsyncState/README.md:

  • Why would one use this library instead of static AsyncLocal<T>? There seem to be the following reasons:
    • The lifetime of values is explicitly controlled; if you call an async function that sets a value, the caller can also access this value. With AsyncLocal<T>, values propagate only to called functions and not to callers of async functions, although you can change that by making the caller set up a holder object and mutating that in the called function.
    • Minimizes the number of async locals in the ExecutionContext, making it cheaper to copy (e.g. during ILogger.BeginScope).
    • Can explicitly reset the state and ensure that the values are no longer rooted by ExecutionContext in any long-lived task.
    • Can use the Microsoft.AspNetCore.AsyncState package to tie the lifetime of values to HttpContext, without the code that uses IAsyncContext<T> directly depending on ASP.NET Core.
  • When to call IAsyncState.Initialize() and IAsyncState.Reset(). The default implementation of IAsyncContext<T> will throw if IAsyncState.Initialize has not been called, but the library itself never calls that.
@Tratcher
Copy link
Member

@geeknoid I can update the readme, but who knows the actual answers to these questions?

@Tratcher Tratcher changed the title Microsoft.Extensions.AsyncState doc suggestions Microsoft.Extensions.AsyncState readme suggestions Oct 24, 2023
@geeknoid
Copy link
Member

@Tratcher Unfortunately, the few guys we had working on this stuff left the company a while ago. I think @davidfowl and @tekian likely know what's what.

@geeknoid
Copy link
Member

Internally, there's also some R9 docs here: https://eng.ms/docs/experiences-devices/r9-sdk/docs/http-processing/asynchronous-state

@KalleOlaviNiemitalo
Copy link
Author

Minimizes the number of async locals in the ExecutionContext, making it cheaper to copy (e.g. during ILogger.BeginScope).

Also, when there is a large number of async values linked to the same ExecutionContext, reading them may be faster via this library than if there were a separate AsyncLocal<T> instance for each, because the library can use the precomputed AsyncStateToken.Index as an array index, instead of ManyElementAsyncLocalValueMap being based on Dictionary<IAsyncLocal, object?> and having to compute the hash bucket index and check for hash collisions…? This library might involve more virtual method calls though, so I don't know which one is actually faster.

@KalleOlaviNiemitalo
Copy link
Author

It might be best to recategorise this library from stable to experimental until you get someone familiar with it so that it can be supported.

@Tratcher Tratcher removed their assignment Nov 13, 2023
@geeknoid geeknoid added enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed untriaged labels Nov 22, 2023
@ghost ghost removed the work in progress 🚧 label Nov 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-fundamentals documentation enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants