Adds MultiCancellationToken

[aeisenberg]

This is a cancellation token that cancels when any of its constituent cancellation tokens are cancelled.

This token is used to fix a bug in Find Definitions. Previously, when clicking CTRL (or CMD on macs) inside a source file in an archive and hovering over a token, this will automatically invoke the definitions finder (in preparation for navigating to the definition). The only way to cancel is to move down to the message popup and click cancel there.

However, this is a bug. What should happen is that if a user moves their mouse away from the token, the operation should cancel.

The underlying problem is that the extension was only listening to the cancellation token from inside getLocationsForUriString the cancellation token used by the Language Server protocol to cancel operations in flight was being ignored.

This fix will ensure we are listening to both cancellation tokens and cancel the query if either are cancelled.

To test this out, go to a source file in a database, press the CTRL or CMD key and hover over a token. Wait for the find definitions popup to appear. Quickly move your mouse away and notice that the popup disappears. Inside the Query Server output view, you should see an indication that the query was cancelled.

In the following screen capture, you can see that hovering over the token kicks off a query and moving away from the token cancels it. (Hmmmm...maybe it's not the best screen capture since the mouse moves pretty fast, but if you squint, you can see it working.)

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[robertbrignull]

I have one comment on the implementation of MultiCancellationToken.

I also tried to test it but I wasn't able to get the CodeQL query to cancel by moving my mouse away from the token. The code looks like it should work, so I'm a little confused. Is there something else I have to do, is should moving my mouse away from its original position be enough? Sadly I can't quite tell what's happening in your screen recording.

[robertbrignull]

I'm not sure we're implementing onCancellationRequested here in line with how it's intended. Maybe this doesn't matter in practice for this usage, but since we're writing a reusable piece of plumbing here it would be nice to do implement it as fully as possible.

As I understand it, the purpose of onCancellationRequested is to be an event that code can listen to and know when the token has been cancelled. Therefore rather than us calling onCancellationRequested on all the child tokens, I believe the flow of information/control should be the other way around and we should instead be listening for one of the child tokens to fire its onCancellationRequested event.

This requires changes to a few different areas of the class so I can't really do this as a "suggestion" block, but I think the code would look like:

export class MultiCancellationToken implements CancellationToken {
  private readonly tokens: CancellationToken[];
  private readonly onCancellationRequestedEvent = new EventEmitter<void>();

  constructor(...tokens: CancellationToken[]) {
    this.tokens = tokens;
    tokens.forEach((t) =>
      t.onCancellationRequested(() => this.onCancellationRequestedEvent.fire()),
    );
  }

  get isCancellationRequested(): boolean {
    return this.tokens.some((t) => t.isCancellationRequested);
  }

  get onCancellationRequested(): Event<any> {
    return this.onCancellationRequestedEvent.event;
  }
}

What do you think?

[aeisenberg]

Thanks. Good suggestion.

Regarding cancellation by mouse hover, the way caching works, the first time the query runs to completion, all other query calls will use the cached results instead of calling it again. Sometimes the query completes quickly so it's hard to get the hover to cancel.

Can you try this on a larger file in the archive?

[robertbrignull]

the first time the query runs to completion, all other query calls will use the cached results instead of calling it again

This is the behaviour I'm seeing, but unfortunately I still can't get it to cancel by breaking the hover. I picked a large file so the query took a few seconds to run. When it was running I could cancel the query by using the progress bar notification, but moving the mouse around seemingly did nothing. Once it had run to completion once it didn't run again and used cached results.

I tried adding in some logging to MultiCancellationToken.isCancellationRequested and it seems that neither of the sub-tokens was considered cancelled. So there's not anything wrong with MultiCancellationToken but there is with the way I'm testing because VS Code isn't cancelling the token when I move the mouse. Could there perhaps be a setting that controls this, so our VS Code instances are behaving differently?

As I said above, the code looks fine to me, so if we can't understand it and it works on your machine I'm happy to believe it and merge the code.

[aeisenberg]

I will verify this again locally. I'll also try to get a second screen recording.

[aeisenberg]

Hmmm...I can't explain why, but when I am using your suggestion, I can't get the operation to cancel when moving away from hovers.

When I revert that change, I can get the cancel to happen, but only if I moved the mouse before evaluation starts happening (which usually takes a second or two). This makes sense since the CLI only checks for cancellation at certain times during evaluation and for shorter queries like this, it may not happen too often.

I'll have to explore some more, but I don't have time right now.

[aeisenberg]

Right...the behaviour we want is when the MultiCancellationToken is canceled, it should forward on the cancellation request to each of the contained tokens. The way it's implemented now, it's the reverse. When any of the contained tokens are canceled, the multi token is also canceled.

Unfortunately, there is no way to cancel a token programmatically. You can only add a listener for cancellation. I'm going to undo your suggested change.

[aeisenberg]

@robertbrignull are you ok with these changes?

[robertbrignull]

are you ok with these changes?

Excitingly, I have just testing it out again and I have managed to replicate the fix and get the definitions query to cancel by moving my mouse! 🎉

the behaviour we want is when the MultiCancellationToken is canceled, it should forward on the cancellation request to each of the contained tokens. The way it's implemented now, it's the reverse. When any of the contained tokens are canceled, the multi token is also canceled.

I'm not sure about this. You also say that there's no programmatic way to cancel tokens, so how would we have the cancelation request propagate in this way? Regardless of the way we describe it, I think what's implemented in this PR currently for the isCancellationRequested method is correct.

For onCancellationRequested I'm still not sure exactly how these tokens are intended to work. But I'm happy to move on with this PR.

[robertbrignull]

(Feel free to do now, ignore, or leave for a followup PR)

Could this just be the constructor of DisposableObject, and avoid adding a new class? The case of passing no args is still allowed and would be a no-op, so it seems like it would be backwards compatible with existing uses of DisposableObject.

[aeisenberg]

DisposableObject is currently abstract, so I'd need to change that, too. But maybe that's not a terrible thing.