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 async operation listener to allow integration tests to wait for server side operations #70359

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Oct 12, 2023

In most cases for vscode integration tests the async operation listener is not necessary. Generally the vscode integration tests work on a client request model, where we ask the server a question and wait for the response.

However there are certain operations where the client does not (or cannot) request and wait for a response. Some examples

  1. LSP notifications sent from the client - these never wait for the request to complete on the server (e.g. option changes).
  2. Project system operations - we don't want to wait and block on the client side for the entire project to load.

This adds an LSP request to hook into the async operation listener on the server side. After the test side loads a project or restores assets on disk or changes options, etc, the test can call this API to wait for the pending operations to complete before continuing.

This is generally a last resort - everything else should make requests and wait for responses.

Client side usage: dotnet/vscode-csharp#6544

@sharwell
Copy link
Member

📝 Tests should also call into this at the end of the test to ensure operations started by one test never extend past the life of one test into the next.

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

Need more info

@@ -67,6 +71,8 @@ public Task HandleNotificationAsync(DidChangeConfigurationParams request, Reques

private async Task RefreshOptionsAsync(CancellationToken cancellationToken)
{
using var _ = _listener.BeginAsyncOperation(nameof(DidChangeConfigurationNotificationHandler));
Copy link
Member

Choose a reason for hiding this comment

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

❓ Why would this be necessary. It appears that all callers of this method await the result, so it does not appear to be possible for this method to be invoked in a fire-and-forget manner.

📝 Asynchronous operation listeners are added at the point where a caller will start an asynchronous operation and then not wait for that operation to complete. This location does not match that characteristic.

Copy link
Member Author

@dibarbet dibarbet Oct 23, 2023

Choose a reason for hiding this comment

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

❓ Why would this be necessary. It appears that all callers of this method await the result, so it does not appear to be possible for this method to be invoked in a fire-and-forget manner.

@sharwell
Still trying to figure out if this is the best solution. But the basic problem is that some things on the client typescript side are sent as LSP notifications - where sending the request can be awaited - but the client does not wait for (or even know, really) when the request completes. These are being sent mostly in VSCode code we have no control over.

This applies to option changes for example - the client sends the server a notification in a fire and forget fashion. So even though the server doesn't fire and forget - the client does. Similar things can happen when restore is invoked - VSCode will send file watcher notifications.

This change allows the client (in integration tests) to query the server to ask if it has finished processing the change.

I'm not 100% certain this is the right way, some other possibilities:

  1. We add a new endpoint (or some how invoke this as a request instead of a notification) to allow the client in integration tests to explicitly call this (again) and wait for the response.
  2. @jasonmalinowski had a good suggestion to instead invoke the listener as a part of the queue itself and track all notifications implicitly - then it would automatically work for listening to things like closed file change events or option changes here.

Copy link
Member

@sharwell sharwell Oct 25, 2023

Choose a reason for hiding this comment

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

I would expect the queue to have the ability to wait for pending operations to complete. Otherwise, the strategy of using our tracking system for all messages (like mentioned in the previous comment) would provide that ability.

@dibarbet dibarbet marked this pull request as draft February 9, 2024 00:31
@jasonmalinowski jasonmalinowski removed their request for review February 9, 2024 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants