-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Use IAsyncEnumerable in NavigateTo OOP calls. #64576
Conversation
storageService, patternName, patternContainer, declaredSymbolInfoKindsSet, | ||
onItemFound, priorityDocumentKeys, cancellationToken).ConfigureAwait(false); | ||
priorityDocumentKeys, cancellationToken).WithCancellation(cancellationToken)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcouv do i need to call .WithCancellation in all these plaes? i'm annotating all async IAsyncEnumerable
methods with the appropriate [EnumeratorCancellation]
attribute, so i'm hping the answer is 'no'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that WithCancellation
is an extension method defined in the MS.VS.Threading assembly, then it certainly isn't necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrusNajmabadi That's correct. .WithCancellation(...)
isn't needed when the method is annotated with [EnumeratorCancellation]
and you're passing the cancellation token.
http://blog.monstuff.com/archives/2019/03/async-enumerables-with-cancellation.html
|
||
await Task.WhenAll(tasks).ConfigureAwait(false); | ||
return builder.ToArray().MergeAsync(cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MergeAsync (which you'll see later in the PR) allows us to take N
IAsyncEnumerables and combine them into a single final stream. the individual values can all be interleaved.
src/Features/Core/Portable/NavigateTo/AbstractNavigateToSearchService.InProcess.cs
Show resolved
Hide resolved
return; | ||
|
||
await ProcessIndexAsync( | ||
documentKey.Id, document: null, patternName, patternContainer, kinds, onItemFound, index, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code was extracted to a helper called ProcessStaleIndexAsync (in another file).
} | ||
|
||
private static async Task ProcessIndexAsync( | ||
private static async IAsyncEnumerable<RoslynNavigateToItem> ProcessStaleIndexAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is the moved method.
} | ||
else | ||
{ | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the deepest part of the stream where the actual values are create and bubble up. everything above this is just passing the values back.
Solution solution, | ||
Document? activeDocument, | ||
IAsyncEnumerable<RoslynNavigateToItem> items, | ||
[EnumeratorCancellation] CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is effectively a .WhereAsync
for IAsyncEnumerable
.
|
||
ValueTask HydrateAsync(Checksum solutionChecksum, CancellationToken cancellationToken); | ||
|
||
public interface ICallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no more remote callback. woot :)
{ | ||
yield break; | ||
} | ||
#pragma warning restore CS1998 // Async method lacks 'await' operators and will run synchronously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the simplest way i can find to just make a singleon empty IAsyncEnumerable. Would be nice to have this in teh BCL :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any efficiency concerns by not doing this more directly? I agree anything else seems like a PITA though to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure what woudl be the "more directly" approach :)
/// <remarks>This helper is useful when doign parallel processing of work where each job returns an <see | ||
/// cref="IAsyncEnumerable{T}"/>, but one final stream is desired as the result.</remarks> | ||
public static IAsyncEnumerable<T> MergeAsync<T>(this IAsyncEnumerable<T>[] enumerables, CancellationToken cancellationToken) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub for auditing this as well. it feels very sane, but i'd still like your eyes.
/// <summary> | ||
/// Workspace created by the remote host that mirrors the corresponding client workspace. | ||
/// </summary> | ||
internal sealed partial class RemoteWorkspace : Workspace | ||
{ | ||
public sealed class PinnedSolution : System.IAsyncDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this portion of the PR is the scariest. Prior to this PR we had the model where remote services pass a simple async callback to do work to RemoteWorkspace. RemoteWorkspace itself pins the solution and invokes the simple callback.
this is no longer simple as we have to support streaming and we have to ensure the solution is pinned while that happens. Once done with the streaming we have to unpin the solution. (or, decrement the ref count on it and unpin iif it goes to zero).
{ | ||
throw ExceptionUtilities.Unreachable(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the bridge to from the existing pattern (which works for all non-streaming cases) to the nwe functionality. It aims to be extremely trivial. specifically, it gets the pinnedSolution, then immediately puts it in a using
so it will clean up. I then runs the client-provided helper to actually do work.
try | ||
{ | ||
var pinnedSolution = await GetPinnedSolutionAsync(assetProvider, solutionChecksum, workspaceVersion, updatePrimaryBranch, cancellationToken).ConfigureAwait(false); | ||
await using (pinnedSolution.ConfigureAwait(false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub the need for the two .ConfigureAwaits
seems... really unfortunate. It's also scary to see the acquisition of the IAsyncDisposable, and then a separate statement before it it actually placed into the using.
Is there a better way to do this?
@@ -13,15 +13,34 @@ | |||
using Microsoft.CodeAnalysis.SolutionCrawler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dibarbet for eyes on the changes in this file as well as you were heavily involved with the last refactoring here.
src/Workspaces/Core/Portable/Shared/Extensions/IAsyncEnumerableExtensions.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Shared/Extensions/IAsyncEnumerableExtensions.cs
Show resolved
Hide resolved
@jasonmalinowski @tmat @dibarbet ptal. thanks! |
// allow reading to complete as well. | ||
Task.WhenAll(tasks).ContinueWith( | ||
t => channel.Writer.Complete(t.Exception), | ||
CancellationToken.None, TaskContinuationOptions.None, TaskScheduler.Default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if TaskContinuationOptions.ExecuteSynchronously makes sense here.
src/Workspaces/Core/Portable/Shared/Extensions/IAsyncEnumerableExtensions.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any obvious issues in the pinned solution bits, just a couple of suggestions for cleanup
src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.PinnedSolution.cs
Outdated
Show resolved
Hide resolved
@dibarbet @jasonmalinowski @tmat ptal. thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pinned solution bits seem fine. Rest also seems OK but someone else should also review those bits :)
src/Workspaces/Remote/ServiceHub/Services/NavigateToSearch/RemoteNavigateToSearchService.cs
Outdated
Show resolved
Hide resolved
…oteNavigateToSearchService.cs Co-authored-by: David Barbet <[email protected]>
@jasonmalinowski ptal :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My signoff here doesn't really count for the pinned solution changes, since I don't know that code well enough to know if anything is really getting worse there. There's some tiny fishy bits, but hopefully somebody else can confirm everything is fine.
Overall wow this is so much nicer, I can't wait to use more IAsyncEnumerable in other places, I think!
await foreach (var item in service.SearchDocumentAsync( | ||
_activeDocument, _searchPattern, _kinds, _activeDocument, cancellationToken)) | ||
{ | ||
await _callback.AddItemAsync(project, item, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep pushing the pattern further up higher, or was this a convenient stopping point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is effectively the top level entrypoint (or close enough to not make moving up any more matter :))
{ | ||
yield break; | ||
} | ||
#pragma warning restore CS1998 // Async method lacks 'await' operators and will run synchronously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any efficiency concerns by not doing this more directly? I agree anything else seems like a PITA though to do.
// Note: passing CancellationToken.None here is intentional/correct. We must complete all the channels to | ||
// allow reading to complete as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does cancellation work? In the cancelled case, is the exception here an OperationCancelledException, or something else? And since Exception here actually returns an aggregate exception, is that going to cause us to wrap exceptions when we don't expect it? Or does .Complete() here do unwrapping of the aggregate exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good question. i'll wait on @stephentoub to explain this because i'm not actually sure if this handled cancellation properly.
From muy reading, you'll get an AggregateException wrapping all the cancellation exceptions. i think the TPL sees through this when completing the writer (and then exposing that on the reader side), so you hopefully just get cancellation on the reading side.
But i'm not sure. @stephentoub for insights.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just debugged this with a little test. The task you get here is:
Note that it is in the canceled state, but .Exception
is null. From teh docs, this is what is said:
If none of the supplied tasks faulted but at least one of them was canceled, the returned task will end in the Canceled state.
If you're waiting on a Task that transitions to the Canceled state, a System.Threading.Tasks.TaskCanceledException exception (wrapped in an AggregateException exception) is thrown. This exception indicates successful cancellation instead of a faulty situation. Therefore, the task's Exception property returns null.
The last line is particularly relevant.
So it seems like task.Exception is null in this case. This is also something you can trivially demonstrate by doing Task.FromCanceled(canceledToken) and checking .Exception on that.
So i think we're all good here. Will comment in next pr.
|
||
// Intentionally do not call GetSolutionAsync here. We do not want the cost of | ||
// synchronizing the solution over to the remote side. Instead, we just directly | ||
// check whatever cached data we have from the previous vs session. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// check whatever cached data we have from the previous vs session. | |
// check whatever cached data we have from the previous VS session. |
I recognize not new, but lowercase "vs" means "versus" to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wfm :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in next pr.
await AbstractNavigateToSearchService.SearchGeneratedDocumentsInCurrentProcessAsync( | ||
project, searchPattern, kinds.ToImmutableHashSet(), callback, cancellationToken).ConfigureAwait(false); | ||
}, cancellationToken); | ||
await foreach (var item in AbstractNavigateToSearchService.SearchGeneratedDocumentsInCurrentProcessAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe dumb question here: why did this need an await + yield versus just returning directly the SearchGeneratedDocumentsInCurrentProcessAsync return? I don't see any other yielding/async but maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct. t his doesn't need await/yield. i can fixup later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in next pr.
src/Workspaces/Remote/ServiceHub/Services/NavigateToSearch/RemoteNavigateToSearchService.cs
Show resolved
Hide resolved
} | ||
} | ||
|
||
#pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd: can we delete the async keyword here, or is this a compiler bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. unfortunately, you must use async
to use yield
to create an IAsyncEnumerbale (not sure why tbh). So you have to add that modifier, then you have to suppress the warning. @jcouv ?
// Ensure we cleanup if an exception occurred. Otherwise, we'll never release this data. | ||
if (exception != null) | ||
await DecrementInFlightCountAsync(inFlightSolution).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intent is "only run this if we didn't throw an exception", them just move it out of the finally block? I'm confused what's going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
catch (Exception ex) when (FatalError.ReportAndPropagateUnlessCanceled(ex, cancellationToken, ErrorSeverity.Critical)) | ||
catch (Exception ex) when ((exception = ex) == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the == null here? Or intentionally a way to force the block to always be false? This is missing an explanatory comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(see earlier comment being unsure why we have a catch/finally at all)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. this is a pattern we have in OOP that tomas likes for ensuring callstacks remain good.
Given @dibarbet signed off on the pinned solution bits but wanting somebody else for everything else, and I'm signing off on everything except the pinned solution bits, I think you're good @CyrusNajmabadi. 😄 |
…syncStreamOOP"" This reverts commit ccef2a9.
Fixes AB#1632606