Skip to content

Properly await calls to ensure exception handling works for sync and async scenarios #81826

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.

Sign up for GitHub

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

Merged
CyrusNajmabadi merged 4 commits into from
Jan 5, 2026
Merged

Properly await calls to ensure exception handling works for sync and async scenarios #81826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e80c1a
Properly await calls to ensure exception handling works for sync and …
CyrusNajmabadi Dec 31, 2025
459aaf7
Fix
CyrusNajmabadi Jan 2, 2026
7a9547a
Merge remote-tracking branch 'upstream/main' into asyncLSP
CyrusNajmabadi Jan 5, 2026
d9c3930
Add docs
CyrusNajmabadi Jan 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/LanguageServer/Protocol/Handler/AbstractRefreshQueue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ protected void EnqueueRefreshNotification(DocumentUri? documentUri)
}
}

private ValueTask FilterLspTrackedDocumentsAsync(
private async ValueTask FilterLspTrackedDocumentsAsync(
LspWorkspaceManager lspWorkspaceManager,
IClientLanguageServerManager notificationManager,
ImmutableSegmentedList<DocumentUri?> documentUris,
Expand All @@ -117,7 +117,11 @@ private ValueTask FilterLspTrackedDocumentsAsync(
{
try
{
return notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken);
// Fire the notification and immediately return. Refresh notifications are server-wide, and are not
// associated with a particular project/document. So once we've sent one, we can stop processing
// entirely.
await notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken).ConfigureAwait(false);
return;
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Jan 5, 2026

Choose a reason for hiding this comment

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

note: this preserves thee old behavior. we were returning. so this would stop after the first hit. but i don't know if that's actually what we want here. note: if i don't return we get a high amount of memory use (presumably because we're issuing the refresh for a ton of cases. given that we dont' pass the docUri in, i'm guessing we do want tehse semantics.

Copy link
Member

@dibarbet dibarbet Jan 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

yes, the old behavior is correct. refresh notifications are typically server-wide, and don't apply to a specific document or project. Once we've sent one, we don't need to send any more as a single request indicates everything (for that feature) is out of date.

}
catch (Exception ex) when (ex is ObjectDisposedException or ConnectionLostException)
{
Expand All @@ -128,7 +132,6 @@ private ValueTask FilterLspTrackedDocumentsAsync(
}

// LSP is already tracking all changed documents so we don't need to send a refresh request.
return ValueTask.CompletedTask;
}

public virtual void Dispose()
Expand Down
8 changes: 3 additions & 5 deletions src/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratorRefreshQueue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,27 +120,25 @@ await newProject.GetDependentVersionAsync(_disposalTokenSource.Token).ConfigureA
}
}

private ValueTask RefreshSourceGeneratedDocumentsAsync(
private async ValueTask RefreshSourceGeneratedDocumentsAsync(
CancellationToken cancellationToken)
{
var hasOpenSourceGeneratedDocuments = _lspWorkspaceManager.GetTrackedLspText().Keys.Any(uri => uri.ParsedUri?.Scheme == SourceGeneratedDocumentUri.Scheme);
if (!hasOpenSourceGeneratedDocuments)
{
// There are no opened source generated documents - we don't need to bother asking the client to refresh anything.
return ValueTask.CompletedTask;
return;
}

try
{
return _notificationManager.SendNotificationAsync(RefreshSourceGeneratedDocumentName, cancellationToken);
await _notificationManager.SendNotificationAsync(RefreshSourceGeneratedDocumentName, cancellationToken).ConfigureAwait(false);
}
catch (Exception ex) when (ex is ObjectDisposedException or ConnectionLostException)
{
// It is entirely possible that we're shutting down and the connection is lost while we're trying to send a notification
// as this runs outside of the guaranteed ordering in the queue. We can safely ignore this exception.
}

return ValueTask.CompletedTask;
}

public void Dispose()
Expand Down
Loading