From 11d19e30190111bde271bf8febc85afe2952d09d Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 9 Feb 2015 17:03:46 -0800 Subject: [PATCH 1/2] Fix issue with cancellation corrupting LS state. The problem here was as follows: 1) Host calls into the LS to do some sort of operation. 2) LS tries to synchronize with the host. 3) During synchronization we attempt to create a new program. 4) Creating the new program causes us to incrementally update some source files. 5) Incrementally updating a source file produces a new source file, and invalidates the old one. 6) *Then* the host asks to cancel this operation. 7) THe synchronization process cancels itself, leaving the LS in an inconsistent state where some of its source files have had their trees updated, but the information about the source file still thinks that we have the previous version. The fix is to not allow cancellation during host synchronization. Once we start, we have to go all the way to completion. --- src/services/services.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/services/services.ts b/src/services/services.ts index 56e68b2f5a1..eda3a4aeff6 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2022,6 +2022,12 @@ module ts { return; } + // IMPORTANT - It is critical from this moment onward that we do not check + // cancellation tokens. We are about to mutate source files from a previous program + // instance. If we cancel midway through, we may end up in an inconsistent state where + // the program points to old source files that have been invalidated because of + // incremental parsing. + var oldSettings = program && program.getCompilerOptions(); var newSettings = hostCache.compilationSettings(); var changesInCompilationSettingsAffectSyntax = oldSettings && oldSettings.target !== newSettings.target; @@ -2056,8 +2062,6 @@ module ts { return; function getOrCreateSourceFile(fileName: string): SourceFile { - cancellationToken.throwIfCancellationRequested(); - // The program is asking for this file, check first if the host can locate it. // If the host can not locate the file, then it does not exist. return undefined // to the program to allow reporting of errors for missing files. @@ -5363,9 +5367,6 @@ module ts { cancellationToken.throwIfCancellationRequested(); var fileContents = sourceFile.text; - - cancellationToken.throwIfCancellationRequested(); - var result: TodoComment[] = []; if (descriptors.length > 0) { From b86ef44e59053e2c41fa40579454ac1933adab61 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 9 Feb 2015 17:24:01 -0800 Subject: [PATCH 2/2] Add assert that clients do not try to call updateSourceFile multiple times on a source file. --- src/compiler/parser.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index a1dc0ef7b1d..06db869ad54 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -736,6 +736,16 @@ module ts { return parseSourceFile(sourceFile.fileName, newText, sourceFile.languageVersion, /*syntaxCursor*/ undefined, /*setNodeParents*/ true) } + // Make sure we're not trying to incrementally update a source file more than once. Once + // we do an update the original source file is considered unusbale from that point onwards. + // + // This is because we do incremental parsing in-place. i.e. we take nodes from the old + // tree and give them new positions and parents. From that point on, trusting the old + // tree at all is not possible as far too much of it may violate invariants. + var incrementalSourceFile = sourceFile; + Debug.assert(!incrementalSourceFile.hasBeenIncrementallyParsed); + incrementalSourceFile.hasBeenIncrementallyParsed = true; + var oldText = sourceFile.text; var syntaxCursor = createSyntaxCursor(sourceFile); @@ -774,7 +784,7 @@ module ts { // // Also, mark any syntax elements that intersect the changed span. We know, up front, // that we cannot reuse these elements. - updateTokenPositionsAndMarkElements(sourceFile, + updateTokenPositionsAndMarkElements(incrementalSourceFile, changeRange.span.start, textSpanEnd(changeRange.span), textSpanEnd(textChangeRangeNewSpan(changeRange)), delta, oldText, newText, aggressiveChecks); // Now that we've set up our internal incremental state just proceed and parse the @@ -815,6 +825,7 @@ module ts { } interface IncrementalNode extends Node, IncrementalElement { + hasBeenIncrementallyParsed: boolean } interface IncrementalNodeArray extends NodeArray, IncrementalElement {