Skip to content

Adds MultiCancellationToken #2885

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
aeisenberg merged 9 commits into from
Oct 4, 2023
Merged

Adds MultiCancellationToken #2885

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e8f68c1
Adds `MultiCancellationToken`
aeisenberg Sep 28, 2023
3ba1712
Update Changelog
aeisenberg Sep 28, 2023
04dfc4e
Move location of multi-cancellation-token
aeisenberg Sep 29, 2023
0fa3cf5
Use `EventEmitter` in `MultiCancellationToken`
aeisenberg Oct 2, 2023
625b3a5
Merge branch 'main' into aeisenberg/multi-token
aeisenberg Oct 3, 2023
ccbe4ee
Merge branch 'main' into aeisenberg/multi-token
aeisenberg Oct 3, 2023
3699f0b
Add `BasicDisposableObject`
aeisenberg Oct 3, 2023
801df7b
Merge branch 'main' into aeisenberg/multi-token
aeisenberg Oct 4, 2023
76ec9e2
Make `DisposableObject` a concrete class
aeisenberg Oct 4, 2023
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
1 change: 1 addition & 0 deletions extensions/ql-vscode/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [UNRELEASED]

- Fix a bug where the query to Find Definitions in database source files would not be cancelled appropriately. [#2885](https://github.com/github/vscode-codeql/pull/2885)
- It is now possible to show the language of query history items using the `%l` specifier in the `codeQL.queryHistory.format` setting. Note that this only works for queries run after this upgrade, and older items will show `unknown` as a language. [#2892](https://github.com/github/vscode-codeql/pull/2892)
- Increase the required version of VS Code to 1.82.0. [#2877](https://github.com/github/vscode-codeql/pull/2877)
- Fix a bug where the query server was restarted twice after configuration changes. [#2884](https://github.com/github/vscode-codeql/pull/2884).
Expand Down
9 changes: 9 additions & 0 deletions extensions/ql-vscode/src/common/disposable-object.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,12 @@ export abstract class DisposableObject implements Disposable {
}
}
}

export class BasicDisposableObject extends DisposableObject {
constructor(...dispoables: Disposable[]) {
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull Oct 4, 2023

Choose a reason for hiding this comment

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

(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.

Copy link
Copy Markdown
Contributor Author

@aeisenberg aeisenberg Oct 4, 2023

Choose a reason for hiding this comment

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

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

super();
for (const d of dispoables) {
this.push(d);
}
}
}
25 changes: 25 additions & 0 deletions extensions/ql-vscode/src/common/vscode/multi-cancellation-token.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { CancellationToken, Disposable } from "vscode";
import { BasicDisposableObject } from "../disposable-object";

/**
* A cancellation token that cancels when any of its constituent
* cancellation tokens are cancelled.
*/
export class MultiCancellationToken implements CancellationToken {
private readonly tokens: CancellationToken[];

constructor(...tokens: CancellationToken[]) {
this.tokens = tokens;
}

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

onCancellationRequested<T>(listener: (e: T) => any): Disposable {
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull Oct 2, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@aeisenberg aeisenberg Oct 2, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull Oct 3, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@aeisenberg aeisenberg Oct 3, 2023

Choose a reason for hiding this comment

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

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

const disposables = this.tokens.map((t) =>
t.onCancellationRequested(listener),
);
return new BasicDisposableObject(...disposables);
}
}
35 changes: 23 additions & 12 deletions extensions/ql-vscode/src/language-support/contextual/template-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@ import {
import { CoreCompletedQuery, QueryRunner } from "../../query-server";
import { AstBuilder } from "../ast-viewer/ast-builder";
import { qlpackOfDatabase } from "../../local-queries";
import { MultiCancellationToken } from "../../common/vscode/multi-cancellation-token";

/**
* Runs templated CodeQL queries to find definitions in
* source-language files. We may eventually want to find a way to
* generalize this to other custom queries, e.g. showing dataflow to
* or from a selected identifier.
*/

export class TemplateQueryDefinitionProvider implements DefinitionProvider {
private cache: CachedOperation<LocationLink[]>;

Expand All @@ -60,11 +62,11 @@ export class TemplateQueryDefinitionProvider implements DefinitionProvider {
async provideDefinition(
document: TextDocument,
position: Position,
_token: CancellationToken,
token: CancellationToken,
): Promise<LocationLink[]> {
const fileLinks = this.shouldUseCache()
? await this.cache.get(document.uri.toString())
: await this.getDefinitions(document.uri.toString());
? await this.cache.get(document.uri.toString(), token)
: await this.getDefinitions(document.uri.toString(), token);

const locLinks: LocationLink[] = [];
for (const link of fileLinks) {
Expand All @@ -79,9 +81,13 @@ export class TemplateQueryDefinitionProvider implements DefinitionProvider {
return !(isCanary() && NO_CACHE_CONTEXTUAL_QUERIES.getValue<boolean>());
}

private async getDefinitions(uriString: string): Promise<LocationLink[]> {
private async getDefinitions(
uriString: string,
token: CancellationToken,
): Promise<LocationLink[]> {
return withProgress(
async (progress, token) => {
async (progress, tokenInner) => {
const multiToken = new MultiCancellationToken(token, tokenInner);
return getLocationsForUriString(
this.cli,
this.qs,
Expand All @@ -90,7 +96,7 @@ export class TemplateQueryDefinitionProvider implements DefinitionProvider {
KeyType.DefinitionQuery,
this.queryStorageDir,
progress,
token,
multiToken,
(src, _dest) => src === uriString,
);
},
Expand Down Expand Up @@ -126,11 +132,11 @@ export class TemplateQueryReferenceProvider implements ReferenceProvider {
document: TextDocument,
position: Position,
_context: ReferenceContext,
_token: CancellationToken,
token: CancellationToken,
): Promise<Location[]> {
const fileLinks = this.shouldUseCache()
? await this.cache.get(document.uri.toString())
: await this.getReferences(document.uri.toString());
? await this.cache.get(document.uri.toString(), token)
: await this.getReferences(document.uri.toString(), token);

const locLinks: Location[] = [];
for (const link of fileLinks) {
Expand All @@ -148,9 +154,14 @@ export class TemplateQueryReferenceProvider implements ReferenceProvider {
return !(isCanary() && NO_CACHE_CONTEXTUAL_QUERIES.getValue<boolean>());
}

private async getReferences(uriString: string): Promise<FullLocationLink[]> {
private async getReferences(
uriString: string,
token: CancellationToken,
): Promise<FullLocationLink[]> {
return withProgress(
async (progress, token) => {
async (progress, tokenInner) => {
const multiToken = new MultiCancellationToken(token, tokenInner);

return getLocationsForUriString(
this.cli,
this.qs,
Expand All @@ -159,7 +170,7 @@ export class TemplateQueryReferenceProvider implements ReferenceProvider {
KeyType.DefinitionQuery,
this.queryStorageDir,
progress,
token,
multiToken,
(src, _dest) => src === uriString,
);
},
Expand Down