Skip to content

Check cancellation token during exportInfoMap generation #45138

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

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

andrewbranch
Copy link
Member

Fixes #44826

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 21, 2021
@andrewbranch andrewbranch requested review from amcasey and sandersn July 21, 2021 18:40
forEachExternalModuleToImportFrom(program, host, /*useAutoImportProvider*/ true, (moduleSymbol, moduleFile, program, isFromPackageJson) => {
if (++moduleCount % 100 === 0) cancellationToken?.throwIfCancellationRequested();
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect the condition to be necessary, since I believe cancellationToken already uses a time-based cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ve definitely heard advice against checking the cancellation token repeatedly, but it could have been outdated or ill-advised. I’ll do a quick perf comparison.

Copy link
Member Author

Choose a reason for hiding this comment

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

TS Server implements a throttled cancellation token, but a LS host could provide a less efficient implementation. I don’t know if we care about that?

Copy link
Member

Choose a reason for hiding this comment

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

I care in principle, but we'd need to review all of our usages to address that (potential) problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know a lot of tree walking functions that switch on node kind select only a few container kinds to check, e.g.

        function checkExpressionWorker(node: Expression | QualifiedName, checkMode: CheckMode | undefined, forceTuple?: boolean): Type {
            const kind = node.kind;
            if (cancellationToken) {
                // Only bother checking on a few construct kinds.  We don't want to be excessively
                // hitting the cancellation token on every node we check.
                switch (kind) {
                    case SyntaxKind.ClassExpression:
                    case SyntaxKind.FunctionExpression:
                    case SyntaxKind.ArrowFunction:
                        cancellationToken.throwIfCancellationRequested();
                }
            }

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, I’m confident that this comment is the reason I had it in my head that I should not check the cancellation token tens of thousands of times in a row in a loop.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow completions request not cancellable while collecting auto-imports
3 participants