Skip to content

Use IAsyncDisposable instead of AsyncLazy<T> for lazy cancellation of final OOP request#61020

Merged
sharwell merged 3 commits intodotnet:mainfrom
sharwell:lazy-cancel
Apr 29, 2022
Merged

Use IAsyncDisposable instead of AsyncLazy<T> for lazy cancellation of final OOP request#61020
sharwell merged 3 commits intodotnet:mainfrom
sharwell:lazy-cancel

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

No description provided.

@ghost ghost added the Area-IDE label Apr 28, 2022
_remoteWorkspace._solutionChecksumToLazySolution.Remove(_solutionChecksum);
}

await _task.NoThrowAwaitable(false);
Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 This line is the key change (AsyncLazy<T> does not await the inner task following its final cancellation)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. please doc :)

if (fromPrimaryBranch)
(newSolution, _) = await this.TryUpdateWorkspaceCurrentSolutionAsync(workspaceVersion, newSolution, cancellationToken).ConfigureAwait(false);
// Actually get the solution, computing it ourselves, or getting the result that another caller was computing.
var newSolution = await refCountedLazySolution.Target.Task.WithCancellation(cancellationToken).ConfigureAwait(false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. would def like a walk through here. this allows teh caller to cancel asap. and then the idea is that the disposable will ensure we dispose the lazy solution (Afaict). however, it's unclear what actually ensures the lazy solution ends up finishing computation and doesn't continue running even once all the callers have returned back out.

_remoteWorkspace._solutionChecksumToLazySolution.Remove(_solutionChecksum);
}

await _task.NoThrowAwaitable(false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i think i got it. this is the critical line it seems. the last one who actually does the dispose will wait on the task being complete. i think this needs doc.s

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz doc why NoThrowAwaitable.

}
}

private sealed class LazySolution : IAsyncDisposable, IDisposable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a generally useful pattern. perhaps could elevate to a common location?

Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it!

@sharwell sharwell marked this pull request as ready for review April 28, 2022 22:18
@sharwell sharwell requested a review from a team as a code owner April 28, 2022 22:18
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Thanks for this. Very clean and clear solution!

@sharwell sharwell enabled auto-merge April 29, 2022 14:31
@sharwell sharwell mentioned this pull request Apr 29, 2022
@sharwell sharwell merged commit 6a8748a into dotnet:main Apr 29, 2022
@ghost ghost added this to the Next milestone Apr 29, 2022
@sharwell sharwell deleted the lazy-cancel branch April 29, 2022 23:52
@Cosifne Cosifne modified the milestones: Next, 17.3 P2 May 31, 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.

3 participants