Skip to content

Commit 7183e2f

Browse files
aasimkhan30Aasim Khan
aasimkhan30
and
Aasim Khan
authored
Fixing connected document parsing in binding queue. (#2617)
* Implement dedicated parsing thread and error handling in LanguageService * refactor(LanguageService): move IncrementalParse and CreateParseThread to virtual methods for better testability * docs(LanguageService): add summary for TryIncrementalParse method to clarify threading behavior * refactor(LanguageService): remove unused parameters from TryIncrementalParse method * test(LanguageServiceTests): add test for handling null ParseResult in ParseAndBind method --------- Co-authored-by: Aasim Khan <aasimkhan@gmail.com>
1 parent 144e6bf commit 7183e2f

2 files changed

Lines changed: 222 additions & 22 deletions

File tree

‎src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs‎

Lines changed: 73 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -911,27 +911,14 @@ public Task<ParseResult> ParseAndBind(ScriptFile scriptFile, ConnectionInfo conn
911911
{
912912
if (connInfo == null || !parseInfo.IsConnected)
913913
{
914-
// parse on separate thread so stack size can be increased
915-
var parseThread = new Thread(() =>
914+
if (TryIncrementalParse(scriptFile.Contents, parseInfo.ParseResult, this.DefaultParseOptions, out ParseResult parseResult))
916915
{
917-
try
918-
{
919-
// parse current SQL file contents to retrieve a list of errors
920-
ParseResult parseResult = Parser.IncrementalParse(
921-
scriptFile.Contents,
922-
parseInfo.ParseResult,
923-
this.DefaultParseOptions);
924-
925-
parseInfo.ParseResult = parseResult;
926-
}
927-
catch (Exception e)
928-
{
929-
// Log the exception but don't rethrow it to prevent parsing errors from crashing SQL Tools Service
930-
Logger.Error(string.Format("An unexpected error occured while parsing: {0}", e.ToString()));
931-
}
932-
}, ConnectedBindingQueue.QueueThreadStackSize);
933-
parseThread.Start();
934-
parseThread.Join();
916+
parseInfo.ParseResult = parseResult;
917+
}
918+
else
919+
{
920+
parseInfo.ParseResult = null;
921+
}
935922
}
936923
else
937924
{
@@ -942,10 +929,15 @@ public Task<ParseResult> ParseAndBind(ScriptFile scriptFile, ConnectionInfo conn
942929
{
943930
try
944931
{
945-
ParseResult parseResult = Parser.IncrementalParse(
932+
if (!TryIncrementalParse(
946933
scriptFile.Contents,
947934
parseInfo.ParseResult,
948-
bindingContext.ParseOptions);
935+
bindingContext.ParseOptions,
936+
out ParseResult parseResult))
937+
{
938+
parseInfo.ParseResult = null;
939+
return null;
940+
}
949941

950942
parseInfo.ParseResult = parseResult;
951943

@@ -997,6 +989,65 @@ public Task<ParseResult> ParseAndBind(ScriptFile scriptFile, ConnectionInfo conn
997989
return parseInfo.ParseResult;
998990
});
999991
}
992+
993+
/// <summary>
994+
/// Runs parse on a separate thread to avoid blocking and crashing the main thread if the parser
995+
/// hangs or crashes.
996+
/// </summary>
997+
private bool TryIncrementalParse(
998+
string sqlText,
999+
ParseResult previousParseResult,
1000+
ParseOptions parseOptions,
1001+
out ParseResult parseResult)
1002+
{
1003+
ParseResult incrementalParseResult = null;
1004+
Exception parseException = null;
1005+
1006+
Thread parseThread = this.CreateParseThread(() =>
1007+
{
1008+
try
1009+
{
1010+
incrementalParseResult = this.IncrementalParse(sqlText, previousParseResult, parseOptions);
1011+
}
1012+
catch (Exception ex)
1013+
{
1014+
parseException = ex;
1015+
}
1016+
});
1017+
1018+
parseThread.Start();
1019+
parseThread.Join();
1020+
parseResult = incrementalParseResult;
1021+
1022+
if (parseException != null)
1023+
{
1024+
Logger.Error($"An unexpected error occured while parsing: {parseException}");
1025+
return false;
1026+
}
1027+
1028+
if (incrementalParseResult == null)
1029+
{
1030+
Logger.Error("Parser returned a null ParseResult.");
1031+
return false;
1032+
}
1033+
1034+
return true;
1035+
}
1036+
1037+
internal virtual ParseResult IncrementalParse(
1038+
string sqlText,
1039+
ParseResult previousParseResult,
1040+
ParseOptions parseOptions)
1041+
{
1042+
return Parser.IncrementalParse(sqlText, previousParseResult, parseOptions);
1043+
}
1044+
1045+
internal virtual Thread CreateParseThread(ThreadStart threadStart)
1046+
{
1047+
Thread thread = new Thread(threadStart, ConnectedBindingQueue.QueueThreadStackSize);
1048+
thread.IsBackground = true;
1049+
return thread;
1050+
}
10001051

10011052
/// <summary>
10021053
/// Runs UpdateLanguageServiceOnConnection as a background task

‎test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/LanguageServiceTests.cs‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@
55

66
#nullable disable
77

8+
using System;
9+
using System.Threading;
10+
using System.Threading.Tasks;
11+
using Microsoft.SqlServer.Management.SqlParser.Common;
12+
using Microsoft.SqlServer.Management.SqlParser.Parser;
813
using Microsoft.SqlTools.ServiceLayer.LanguageServices;
914
using Microsoft.SqlTools.ServiceLayer.LanguageServices.Completion;
1015
using Microsoft.SqlTools.ServiceLayer.LanguageServices.Contracts;
@@ -19,6 +24,27 @@ namespace Microsoft.SqlTools.ServiceLayer.UnitTests.LanguageServer
1924
/// </summary>
2025
public class LanguageServiceTests
2126
{
27+
private sealed class TestLanguageService : LanguageService
28+
{
29+
internal Func<string, ParseResult, ParseOptions, ParseResult> IncrementalParseOverride { get; set; }
30+
31+
internal Func<ThreadStart, Thread> CreateParseThreadOverride { get; set; }
32+
33+
internal override ParseResult IncrementalParse(string sqlText, ParseResult previousParseResult, ParseOptions parseOptions)
34+
{
35+
return this.IncrementalParseOverride != null
36+
? this.IncrementalParseOverride(sqlText, previousParseResult, parseOptions)
37+
: base.IncrementalParse(sqlText, previousParseResult, parseOptions);
38+
}
39+
40+
internal override Thread CreateParseThread(ThreadStart threadStart)
41+
{
42+
return this.CreateParseThreadOverride != null
43+
? this.CreateParseThreadOverride(threadStart)
44+
: base.CreateParseThread(threadStart);
45+
}
46+
}
47+
2248
/// <summary>
2349
/// Verify that the SQL parser correctly detects errors in text
2450
/// </summary>
@@ -186,5 +212,128 @@ public void GetDefaultCompletionListWithMatchesTest()
186212
CompletionItem[] result = AutoCompleteHelper.GetDefaultCompletionItems(scriptDocumentInfo, false);
187213
Assert.AreEqual(1, result.Length);
188214
}
215+
216+
[Test]
217+
public async Task ParseAndBindConnectedPathUsesDedicatedParseThreadAndClearsFailedParseState()
218+
{
219+
TestLanguageService service = new TestLanguageService();
220+
ConnectedBindingQueue bindingQueue = new ConnectedBindingQueue(false);
221+
service.BindingQueue = bindingQueue;
222+
223+
var scriptFile = new ScriptFile();
224+
scriptFile.SetFileContents("SELECT 1");
225+
226+
var parseOptions = new ParseOptions(
227+
batchSeparator: LanguageService.DefaultBatchSeperator,
228+
isQuotedIdentifierSet: true,
229+
compatibilityLevel: DatabaseCompatibilityLevel.Current,
230+
transactSqlVersion: TransactSqlVersion.Current);
231+
232+
ScriptParseInfo scriptParseInfo = new ScriptParseInfo
233+
{
234+
IsConnected = true,
235+
ConnectionKey = "test-connection-key",
236+
ParseResult = Parser.IncrementalParse("SELECT 1", null, parseOptions)
237+
};
238+
239+
service.AddOrUpdateScriptParseInfo(scriptFile.ClientUri, scriptParseInfo);
240+
241+
ConnectedBindingContext bindingContext = new ConnectedBindingContext
242+
{
243+
IsConnected = false
244+
};
245+
246+
bindingQueue.BindingContextMap.TryAdd(scriptParseInfo.ConnectionKey, bindingContext);
247+
bindingQueue.BindingContextTasks.TryAdd(bindingContext, Task.FromResult(0));
248+
249+
int callingThreadId = Environment.CurrentManagedThreadId;
250+
int parserThreadId = callingThreadId;
251+
bool dedicatedThreadCreated = false;
252+
253+
service.CreateParseThreadOverride = threadStart =>
254+
{
255+
dedicatedThreadCreated = true;
256+
return new Thread(threadStart);
257+
};
258+
259+
service.IncrementalParseOverride = (sqlText, previousParseResult, options) =>
260+
{
261+
parserThreadId = Environment.CurrentManagedThreadId;
262+
throw new InvalidOperationException("parser fault");
263+
};
264+
265+
try
266+
{
267+
ParseResult parseResult = await service.ParseAndBind(scriptFile, TestObjects.GetTestConnectionInfo());
268+
269+
Assert.IsNull(parseResult);
270+
Assert.IsNull(scriptParseInfo.ParseResult);
271+
Assert.IsTrue(dedicatedThreadCreated);
272+
Assert.AreNotEqual(callingThreadId, parserThreadId);
273+
}
274+
finally
275+
{
276+
bindingQueue.StopQueueProcessor(1000);
277+
bindingQueue.Dispose();
278+
}
279+
}
280+
281+
[Test]
282+
public async Task ParseAndBindConnectedPathClearsParseStateWhenParserReturnsNull()
283+
{
284+
TestLanguageService service = new TestLanguageService();
285+
ConnectedBindingQueue bindingQueue = new ConnectedBindingQueue(false);
286+
service.BindingQueue = bindingQueue;
287+
288+
var scriptFile = new ScriptFile();
289+
scriptFile.SetFileContents("SELECT 1");
290+
291+
var parseOptions = new ParseOptions(
292+
batchSeparator: LanguageService.DefaultBatchSeperator,
293+
isQuotedIdentifierSet: true,
294+
compatibilityLevel: DatabaseCompatibilityLevel.Current,
295+
transactSqlVersion: TransactSqlVersion.Current);
296+
297+
ScriptParseInfo scriptParseInfo = new ScriptParseInfo
298+
{
299+
IsConnected = true,
300+
ConnectionKey = "test-connection-key",
301+
ParseResult = Parser.IncrementalParse("SELECT 1", null, parseOptions)
302+
};
303+
304+
service.AddOrUpdateScriptParseInfo(scriptFile.ClientUri, scriptParseInfo);
305+
306+
ConnectedBindingContext bindingContext = new ConnectedBindingContext
307+
{
308+
IsConnected = false
309+
};
310+
311+
bindingQueue.BindingContextMap.TryAdd(scriptParseInfo.ConnectionKey, bindingContext);
312+
bindingQueue.BindingContextTasks.TryAdd(bindingContext, Task.FromResult(0));
313+
314+
bool dedicatedThreadCreated = false;
315+
316+
service.CreateParseThreadOverride = threadStart =>
317+
{
318+
dedicatedThreadCreated = true;
319+
return new Thread(threadStart);
320+
};
321+
322+
service.IncrementalParseOverride = (sqlText, previousParseResult, options) => null;
323+
324+
try
325+
{
326+
ParseResult parseResult = await service.ParseAndBind(scriptFile, TestObjects.GetTestConnectionInfo());
327+
328+
Assert.IsNull(parseResult);
329+
Assert.IsNull(scriptParseInfo.ParseResult);
330+
Assert.IsTrue(dedicatedThreadCreated);
331+
}
332+
finally
333+
{
334+
bindingQueue.StopQueueProcessor(1000);
335+
bindingQueue.Dispose();
336+
}
337+
}
189338
}
190339
}

0 commit comments

Comments
 (0)