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

Add group cancellation support to AsyncLazy<T> value factory #952

Open
AArnott opened this issue Nov 16, 2021 · 2 comments
Open

Add group cancellation support to AsyncLazy<T> value factory #952

AArnott opened this issue Nov 16, 2021 · 2 comments
Assignees

Comments

@AArnott
Copy link
Member

AArnott commented Nov 16, 2021

Is your feature request related to a problem? Please describe.

AsyncLazy<T> currently never cancels its value factory even if all callers of GetValue or GetValueAsync have canceled their token.
While this makes for a simple value factory that never cancels and only runs at most once, it can be inefficient when all callers lost interest and the value factory no longer needs to run.

Describe the solution you'd like

Add support for a value factory that takes CancellationToken as a parameter.
When all outstanding GetValue and GetValueAsync callers cancel their own tokens, cancel the value factory.
The value factory is subject to re-invocation only after it throws OperationCanceledException as a result of a canceled token passed to it. If a new GetValueAsync caller comes along while a prior the value factory invocation is still running, that caller must await the completion of that value factory before invoking it again. If the value factory completes successfully, use (and retain) its value. If the value factory cancels, GetValueAsync may re-invoke the factory.

The invariants would be:

  1. A value factory that takes no CancellationToken would never be invoked more than once.
  2. A cancelable value factory will never be invoked concurrently with itself.
  3. A cancelable value factory will never be invoked more than once except after the prior call throws OperationCanceledException and the CancellationToken we provided them is canceled. Note in this check we do not compare the token with OperationCanceledException.CancellationToken because the implementation detail of the value factory may have combined our token with another so its identity may have changed even though it canceled ultimately in response to our token.

Consider exposing this additional functionality via a static factory method on AsyncLazy<T> instead of on a public constructor if it means we can avoid adding fields to the AsyncLazy<T> class, since this class is used a lot and each added field will add memory pressure to apps that use it, such as VS.

Describe alternatives you've considered

We considered running the value factory concurrently with itself after a prior invocation was canceled in order to expedite the result to a subsequent caller. This was rejected for a few reasons:

  1. Writing a value factory that is concurrency-safe seems like a high bar that would lead to buggy code in those that use AsyncLazy<T>.
  2. A subsequent caller that awaits the "canceled" value factory before invoking it again may in fact find that the initial invocation completed successfully rather than responding to the cancellation, in which case the fastest and most efficient thing was in fact to wait for the first invocation to complete.
  3. Running the value factory concurrently may end up with multiple values produced. If the values require disposal or the factory had side-effects, this could result in leaks or other undesirable behavior.

Additional context

This design was hashed out with @matteo-prosperi and @sealslicer.

@CyrusNajmabadi
Copy link

This would be beneficial. TypeScript, for example, uses AsyncLazy but cannot cancel the work that it does, even if there are no interested parties who care. This can definitely lead to more work than necessary happening on the server side.

An example of this is what happens when a LightBulb comes up in VS. As the user arrows down through the items, teh preview window attempts to come up to show what that refactoring/fix would do. That ends up kicking off the work to compute that information, which cannot be canceled, even if the user moves to the next item. This is not ideal as it can force a lot of work on the server that contends with the actual work that is most important to the user.

tagging @MariaSolOs

@AArnott AArnott self-assigned this Mar 9, 2023
@AArnott
Copy link
Member Author

AArnott commented Mar 9, 2023

I've started working on this. Since it doesn't appear to be urgent, but would be useful, it's one of my 'backburner' tasks. Let me know if this becomes urgent.

Notes to self:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants