Skip to content

Commit a37053f

Browse files
jramsayjramsay
committed
Addressing CR comments
- Adding a throttle - Refactor - Navbar reset onCancel
1 parent 3f198e6 commit a37053f

File tree

8 files changed

+137
-102
lines changed

8 files changed

+137
-102
lines changed

‎src/compiler/program.ts

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,20 @@ namespace ts {
7272
return getNormalizedPathFromPathComponents(commonPathComponents);
7373
}
7474

75+
/* @internal */
76+
export function runWithCancellationToken<T>(func: () => T, onCancel?: () => void): T {
77+
try {
78+
return func();
79+
}
80+
catch (e) {
81+
if (e instanceof OperationCanceledException && onCancel) {
82+
// We were canceled while performing the operation.
83+
onCancel();
84+
}
85+
throw e;
86+
}
87+
}
88+
7589
interface OutputFingerprint {
7690
hash: string;
7791
byteOrderMark: boolean;
@@ -873,29 +887,19 @@ namespace ts {
873887
return sourceFile.parseDiagnostics;
874888
}
875889

876-
function runWithCancellationToken<T>(func: () => T): T {
877-
try {
878-
return func();
879-
}
880-
catch (e) {
881-
if (e instanceof OperationCanceledException) {
882-
// We were canceled while performing the operation. Because our type checker
883-
// might be a bad state, we need to throw it away.
884-
//
885-
// Note: we are overly aggressive here. We do not actually *have* to throw away
886-
// the "noDiagnosticsTypeChecker". However, for simplicity, i'd like to keep
887-
// the lifetimes of these two TypeCheckers the same. Also, we generally only
888-
// cancel when the user has made a change anyways. And, in that case, we (the
889-
// program instance) will get thrown away anyways. So trying to keep one of
890-
// these type checkers alive doesn't serve much purpose.
891-
noDiagnosticsTypeChecker = undefined;
892-
diagnosticsProducingTypeChecker = undefined;
893-
}
894-
895-
throw e;
896-
}
890+
function onCancel() {
891+
// Because our type checker might be a bad state, we need to throw it away.
892+
// Note: we are overly aggressive here. We do not actually *have* to throw away
893+
// the "noDiagnosticsTypeChecker". However, for simplicity, i'd like to keep
894+
// the lifetimes of these two TypeCheckers the same. Also, we generally only
895+
// cancel when the user has made a change anyways. And, in that case, we (the
896+
// program instance) will get thrown away anyways. So trying to keep one of
897+
// these type checkers alive doesn't serve much purpose.
898+
noDiagnosticsTypeChecker = undefined;
899+
diagnosticsProducingTypeChecker = undefined;
897900
}
898901

902+
899903
function getSemanticDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] {
900904
return runWithCancellationToken(() => {
901905
const typeChecker = getDiagnosticsProducingTypeChecker();
@@ -910,7 +914,7 @@ namespace ts {
910914
const programDiagnosticsInFile = programDiagnostics.getDiagnostics(sourceFile.fileName);
911915

912916
return bindDiagnostics.concat(checkDiagnostics, fileProcessingDiagnosticsInFile, programDiagnosticsInFile);
913-
});
917+
}, onCancel);
914918
}
915919

916920
function getJavaScriptSyntacticDiagnosticsForFile(sourceFile: SourceFile): Diagnostic[] {
@@ -1089,15 +1093,15 @@ namespace ts {
10891093
function createDiagnosticForNode(node: Node, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): Diagnostic {
10901094
return createDiagnosticForNodeInSourceFile(sourceFile, node, message, arg0, arg1, arg2);
10911095
}
1092-
});
1096+
}, onCancel);
10931097
}
10941098

10951099
function getDeclarationDiagnosticsWorker(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] {
10961100
return runWithCancellationToken(() => {
10971101
const resolver = getDiagnosticsProducingTypeChecker().getEmitResolver(sourceFile, cancellationToken);
10981102
// Don't actually write any files since we're just getting diagnostics.
10991103
return ts.getDeclarationDiagnostics(getEmitHost(noop), resolver, sourceFile);
1100-
});
1104+
}, onCancel);
11011105
}
11021106

11031107
function getDeclarationDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] {

‎src/compiler/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2262,6 +2262,8 @@
22622262

22632263
/** @throws OperationCanceledException if isCancellationRequested is true */
22642264
throwIfCancellationRequested(): void;
2265+
2266+
throttleWaitMilliseconds?: number;
22652267
}
22662268

22672269
export interface Program extends ScriptReferenceHost {

‎src/harness/unittests/tsserverProjectSystem.ts

Lines changed: 50 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -547,6 +547,45 @@ namespace ts.projectSystem {
547547
readonly getEnvironmentVariable = notImplemented;
548548
}
549549

550+
export class TestServerCancellationToken implements server.ServerCancellationToken {
551+
private currentId = -1;
552+
private requestToCancel = -1;
553+
private isCancellationRequestedCount = 0;
554+
555+
constructor(private cancelAfterRequest = 0) {
556+
}
557+
558+
get throttleWaitMilliseconds() {
559+
// For testing purposes disable the throttle
560+
return 0;
561+
}
562+
563+
setRequest(requestId: number) {
564+
this.currentId = requestId;
565+
}
566+
567+
setRequestToCancel(requestId: number) {
568+
this.resetToken();
569+
this.requestToCancel = requestId;
570+
}
571+
572+
resetRequest(requestId: number) {
573+
assert.equal(requestId, this.currentId, "unexpected request id in cancellation")
574+
this.currentId = undefined;
575+
}
576+
577+
isCancellationRequested() {
578+
this.isCancellationRequestedCount++;
579+
return this.requestToCancel === this.currentId && this.isCancellationRequestedCount >= this.cancelAfterRequest;
580+
}
581+
582+
resetToken() {
583+
this.currentId = -1;
584+
this.isCancellationRequestedCount = 0;
585+
this.requestToCancel = -1;
586+
}
587+
}
588+
550589
export function makeSessionRequest<T>(command: string, args: T) {
551590
const newRequest: protocol.Request = {
552591
seq: 0,
@@ -3324,22 +3363,7 @@ namespace ts.projectSystem {
33243363
})
33253364
};
33263365

3327-
let requestToCancel = -1;
3328-
const cancellationToken: server.ServerCancellationToken = (function(){
3329-
let currentId: number;
3330-
return <server.ServerCancellationToken>{
3331-
setRequest(requestId) {
3332-
currentId = requestId;
3333-
},
3334-
resetRequest(requestId) {
3335-
assert.equal(requestId, currentId, "unexpected request id in cancellation")
3336-
currentId = undefined;
3337-
},
3338-
isCancellationRequested() {
3339-
return requestToCancel === currentId;
3340-
}
3341-
}
3342-
})();
3366+
const cancellationToken = new TestServerCancellationToken();
33433367
const host = createServerHost([f1, config]);
33443368
const session = createSession(host, /*typingsInstaller*/ undefined, () => {}, cancellationToken);
33453369
{
@@ -3374,13 +3398,13 @@ namespace ts.projectSystem {
33743398
host.clearOutput();
33753399

33763400
// cancel previously issued Geterr
3377-
requestToCancel = getErrId;
3401+
cancellationToken.setRequestToCancel(getErrId);
33783402
host.runQueuedTimeoutCallbacks();
33793403

33803404
assert.equal(host.getOutput().length, 1, "expect 1 message");
33813405
verifyRequestCompleted(getErrId, 0);
33823406

3383-
requestToCancel = -1;
3407+
cancellationToken.resetToken();
33843408
}
33853409
{
33863410
const getErrId = session.getNextSeq();
@@ -3397,12 +3421,12 @@ namespace ts.projectSystem {
33973421
assert.equal(e1.event, "syntaxDiag");
33983422
host.clearOutput();
33993423

3400-
requestToCancel = getErrId;
3424+
cancellationToken.setRequestToCancel(getErrId);
34013425
host.runQueuedImmediateCallbacks();
34023426
assert.equal(host.getOutput().length, 1, "expect 1 message");
34033427
verifyRequestCompleted(getErrId, 0);
34043428

3405-
requestToCancel = -1;
3429+
cancellationToken.resetToken();
34063430
}
34073431
{
34083432
const getErrId = session.getNextSeq();
@@ -3425,7 +3449,7 @@ namespace ts.projectSystem {
34253449
assert.equal(e2.event, "semanticDiag");
34263450
verifyRequestCompleted(getErrId, 1);
34273451

3428-
requestToCancel = -1;
3452+
cancellationToken.resetToken();
34293453
}
34303454
{
34313455
const getErr1 = session.getNextSeq();
@@ -3472,26 +3496,8 @@ namespace ts.projectSystem {
34723496
})
34733497
};
34743498

3475-
let requestToCancel = -1;
3476-
let isCancellationRequestedCount = 0;
3477-
let cancelAfterRequest = 3;
34783499
let operationCanceledExceptionThrown = false;
3479-
const cancellationToken: server.ServerCancellationToken = (function () {
3480-
let currentId: number;
3481-
return <server.ServerCancellationToken>{
3482-
setRequest(requestId) {
3483-
currentId = requestId;
3484-
},
3485-
resetRequest(requestId) {
3486-
assert.equal(requestId, currentId, "unexpected request id in cancellation")
3487-
currentId = undefined;
3488-
},
3489-
isCancellationRequested() {
3490-
isCancellationRequestedCount++;
3491-
return requestToCancel === currentId && isCancellationRequestedCount >= cancelAfterRequest;
3492-
}
3493-
}
3494-
})();
3500+
const cancellationToken = new TestServerCancellationToken(/*cancelAfterRequest*/ 3);
34953501
const host = createServerHost([f1, config]);
34963502
const session = createSession(host, /*typingsInstaller*/ undefined, () => { }, cancellationToken);
34973503
{
@@ -3529,17 +3535,17 @@ namespace ts.projectSystem {
35293535
// Set the next request to be cancellable
35303536
// The cancellation token will cancel the request the third time
35313537
// isCancellationRequested() is called.
3532-
requestToCancel = session.getNextSeq();
3533-
isCancellationRequestedCount = 0;
3538+
cancellationToken.setRequestToCancel(session.getNextSeq());
35343539
operationCanceledExceptionThrown = false;
35353540

35363541
try {
35373542
session.executeCommandSeq(request);
3538-
} catch (e) {
3543+
}
3544+
catch (e) {
35393545
assert(e instanceof OperationCanceledException);
35403546
operationCanceledExceptionThrown = true;
35413547
}
3542-
assert(operationCanceledExceptionThrown);
3548+
assert(operationCanceledExceptionThrown, "Operation Canceled Exception not thrown for request: " + JSON.stringify(request));
35433549
}
35443550
});
35453551
});

‎src/services/navigationBar.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,21 +14,24 @@ namespace ts.NavigationBar {
1414
indent: number; // # of parents
1515
}
1616

17-
export function getNavigationBarItems(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationBarItem[] {
17+
export function getNavigationBarItems(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): NavigationBarItem[] {
18+
curCancellationToken = cancellationToken;
1819
curSourceFile = sourceFile;
19-
const result = map(topLevelItems(rootNavigationBarNode(sourceFile, cancellationToken)), convertToTopLevelItem);
20+
const result = runWithCancellationToken(() => map(topLevelItems(rootNavigationBarNode(sourceFile)), convertToTopLevelItem), () => curSourceFile = undefined);
2021
curSourceFile = undefined;
2122
return result;
2223
}
2324

24-
export function getNavigationTree(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationTree {
25+
export function getNavigationTree(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): NavigationTree {
26+
curCancellationToken = cancellationToken;
2527
curSourceFile = sourceFile;
26-
const result = convertToTree(rootNavigationBarNode(sourceFile, cancellationToken));
28+
const result = runWithCancellationToken(() => convertToTree(rootNavigationBarNode(sourceFile)), () => curSourceFile = undefined);
2729
curSourceFile = undefined;
2830
return result;
2931
}
3032

3133
// Keep sourceFile handy so we don't have to search for it every time we need to call `getText`.
34+
let curCancellationToken: ThrottledCancellationToken;
3235
let curSourceFile: SourceFile;
3336
function nodeText(node: Node): string {
3437
return node.getText(curSourceFile);
@@ -55,12 +58,12 @@ namespace ts.NavigationBar {
5558
const parentsStack: NavigationBarNode[] = [];
5659
let parent: NavigationBarNode;
5760

58-
function rootNavigationBarNode(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationBarNode {
61+
function rootNavigationBarNode(sourceFile: SourceFile): NavigationBarNode {
5962
Debug.assert(!parentsStack.length);
6063
const root: NavigationBarNode = { node: sourceFile, additionalNodes: undefined, parent: undefined, children: undefined, indent: 0 };
6164
parent = root;
6265
for (const statement of sourceFile.statements) {
63-
addChildrenRecursively(statement, cancellationToken);
66+
addChildrenRecursively(statement, curCancellationToken);
6467
}
6568
endNode();
6669
Debug.assert(!parent && !parentsStack.length);
@@ -110,7 +113,7 @@ namespace ts.NavigationBar {
110113
}
111114

112115
/** Look for navigation bar items in node's subtree, adding them to the current `parent`. */
113-
function addChildrenRecursively(node: Node, cancellationToken: CancellationToken): void {
116+
function addChildrenRecursively(node: Node, cancellationToken: ThrottledCancellationToken): void {
114117
function addChildrenRecursively(node: Node): void {
115118
cancellationToken.throwIfCancellationRequested();
116119

‎src/services/outliningElementsCollector.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/* @internal */
22
namespace ts.OutliningElementsCollector {
3-
export function collectElements(sourceFile: SourceFile, cancellationToken: CancellationToken): OutliningSpan[] {
3+
export function collectElements(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): OutliningSpan[] {
44
const elements: OutliningSpan[] = [];
55
const collapseText = "...";
66

0 commit comments

Comments
 (0)