From 4b66203dfc60083f60c82725c3586d854ee4e99f Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Thu, 19 Oct 2017 10:42:26 -0700 Subject: [PATCH] Fix tools service crash + improve server edition naming (#506) * Fix https://github.com/Microsoft/vscode-mssql/issues/986 and add better server edition naming - Fixed a number of issues related to the binding queue, specifically the fact that it didn't capture exceptions that occurred on the binding thread. This caused the thread to crash the service. - The root cause of the error was when you get a connection error during init of the SmoMetadataProvider which threw an exception. Because of this no Binder was created, and the code would null ref later during processing - Added logic to handle null ref issue and other related code - Added a unit test for the new error handling path, and supported still returning some value in this case - Separately, have a fix for an issue where the edition is shown as "SQL Azure" for all Azure connections. Fixing to be "Azure SQL DB" for this case and handle when the database reports as DW or Stretch instead. This maps better to users concept of what the edition should be and avoids returning what is an outdated term. * Messed up the test - fixing this by returning the expected return object --- .../Connection/ConnectionService.cs | 28 +++- .../LanguageServices/BindingQueue.cs | 41 +++++- .../Completion/CompletionService.cs | 5 + .../LanguageServices/ConnectedBindingQueue.cs | 1 + .../LanguageServices/LanguageService.cs | 82 +++++++----- .../LanguageServices/QueueItem.cs | 7 + .../Localization/sr.cs | 33 +++++ .../Localization/sr.resx | 12 ++ .../Localization/sr.strings | 5 + .../Localization/sr.xlf | 15 +++ .../Scripting/ScriptingService.cs | 4 +- .../sr.es.resx | 123 ------------------ .../LanguageServer/PeekDefinitionTests.cs | 1 + .../LanguageServer/BindingQueueTests.cs | 30 +++++ 14 files changed, 222 insertions(+), 165 deletions(-) delete mode 100644 src/Microsoft.SqlTools.ServiceLayer/sr.es.resx diff --git a/src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs b/src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs index ef2e59d2..2d7acbde 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs @@ -29,6 +29,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection public class ConnectionService { public const string AdminConnectionPrefix = "ADMIN:"; + private const string SqlAzureEdition = "SQL Azure"; /// /// Singleton service instance @@ -456,7 +457,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection EngineEditionId = serverInfo.EngineEditionId, ServerVersion = serverInfo.ServerVersion, ServerLevel = serverInfo.ServerLevel, - ServerEdition = serverInfo.ServerEdition, + ServerEdition = MapServerEdition(serverInfo), IsCloud = serverInfo.IsCloud, AzureVersion = serverInfo.AzureVersion, OsVersion = serverInfo.OsVersion, @@ -474,6 +475,31 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection return response; } + private string MapServerEdition(ReliableConnectionHelper.ServerInfo serverInfo) + { + string serverEdition = serverInfo.ServerEdition; + if (string.IsNullOrWhiteSpace(serverEdition)) + { + return string.Empty; + } + if (SqlAzureEdition.Equals(serverEdition, StringComparison.OrdinalIgnoreCase)) + { + switch(serverInfo.EngineEditionId) + { + case (int) DatabaseEngineEdition.SqlDataWarehouse: + serverEdition = SR.AzureSqlDwEdition; + break; + case (int) DatabaseEngineEdition.SqlStretchDatabase: + serverEdition = SR.AzureSqlStretchEdition; + break; + default: + serverEdition = SR.AzureSqlDbEdition; + break; + } + } + return serverEdition; + } + /// /// Tries to create and open a connection with the given ConnectParams. /// diff --git a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/BindingQueue.cs b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/BindingQueue.cs index b8584e77..b57f960f 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/BindingQueue.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/BindingQueue.cs @@ -9,6 +9,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.SqlTools.Utility; using System.Linq; +using Microsoft.SqlTools.ServiceLayer.Utility; namespace Microsoft.SqlTools.ServiceLayer.LanguageServices { @@ -64,6 +65,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices string key, Func bindOperation, Func timeoutOperation = null, + Func errorHandler = null, int? bindingTimeout = null, int? waitForLockTimeout = null) { @@ -78,6 +80,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices Key = key, BindOperation = bindOperation, TimeoutOperation = timeoutOperation, + ErrorHandler = errorHandler, BindingTimeout = bindingTimeout, WaitForLockTimeout = waitForLockTimeout }; @@ -92,6 +95,23 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices return queueItem; } + /// + /// Checks if a particular binding context is connected or not + /// + /// + public bool IsBindingContextConnected(string key) + { + lock (this.bindingContextLock) + { + IBindingContext context; + if (this.BindingContextMap.TryGetValue(key, out context)) + { + return context.IsConnected; + } + return false; + } + } + /// /// Gets or creates a binding context for the provided context key /// @@ -270,9 +290,20 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices // run the operation in a separate thread var bindThread = new Thread(() => { - result = queueItem.BindOperation( - bindingContext, - cancelToken.Token); + try + { + result = queueItem.BindOperation( + bindingContext, + cancelToken.Token); + } + catch (Exception ex) + { + Logger.Write(LogLevel.Error, "Unexpected exception on the binding queue: " + ex.ToString()); + if (queueItem.ErrorHandler != null) + { + result = queueItem.ErrorHandler(ex); + } + } }, BindingQueue.QueueThreadStackSize); bindThread.Start(); @@ -282,7 +313,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices queueItem.Result = result; } else - { + { cancelToken.Cancel(); // if the task didn't complete then call the timeout callback @@ -298,7 +329,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices // wait for the operation to complete before releasing the lock bindThread.Join(); bindingContext.BindingLock.Set(); - }); + }).ContinueWithOnFaulted(t => Logger.Write(LogLevel.Error, "Binding queue threw exception " + t.Exception.ToString())); } } catch (Exception ex) diff --git a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/Completion/CompletionService.cs b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/Completion/CompletionService.cs index cf7eb6c3..9de595d3 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/Completion/CompletionService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/Completion/CompletionService.cs @@ -104,6 +104,11 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices.Completion { // return the default list if the connected bind fails return CreateDefaultCompletionItems(scriptParseInfo, scriptDocumentInfo, useLowerCaseSuggestions); + }, + errorHandler: ex => + { + // return the default list if an unexpected exception occurs + return CreateDefaultCompletionItems(scriptParseInfo, scriptDocumentInfo, useLowerCaseSuggestions); }); return queueItem; } diff --git a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/ConnectedBindingQueue.cs b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/ConnectedBindingQueue.cs index 1a427251..ce48c512 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/ConnectedBindingQueue.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/ConnectedBindingQueue.cs @@ -27,6 +27,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices string key, Func bindOperation, Func timeoutOperation = null, + Func errorHandler = null, int? bindingTimeout = null, int? waitForLockTimeout = null); } diff --git a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs index 96fe16cd..d19f24c9 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs @@ -794,10 +794,13 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices List parseResults = new List(); parseResults.Add(parseResult); - bindingContext.Binder.Bind( - parseResults, - connInfo.ConnectionDetails.DatabaseName, - BindMode.Batch); + if (bindingContext.IsConnected && bindingContext.Binder != null) + { + bindingContext.Binder.Bind( + parseResults, + connInfo.ConnectionDetails.DatabaseName, + BindMode.Batch); + } } catch (ConnectionException) { @@ -856,8 +859,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices try { scriptInfo.ConnectionKey = this.BindingQueue.AddConnectionContext(info, "languageService"); - scriptInfo.IsConnected = true; - + scriptInfo.IsConnected = this.BindingQueue.IsBindingContextConnected(scriptInfo.ConnectionKey); } catch (Exception ex) { @@ -917,40 +919,42 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices ParseResult parseResult = Parser.Parse( "select ", bindingContext.ParseOptions); + if (bindingContext.IsConnected && bindingContext.Binder != null) + { + List parseResults = new List(); + parseResults.Add(parseResult); + bindingContext.Binder.Bind( + parseResults, + info.ConnectionDetails.DatabaseName, + BindMode.Batch); - List parseResults = new List(); - parseResults.Add(parseResult); - bindingContext.Binder.Bind( - parseResults, - info.ConnectionDetails.DatabaseName, - BindMode.Batch); + // get the completion list from SQL Parser + var suggestions = Resolver.FindCompletions( + parseResult, 1, 8, + bindingContext.MetadataDisplayInfoProvider); - // get the completion list from SQL Parser - var suggestions = Resolver.FindCompletions( - parseResult, 1, 8, - bindingContext.MetadataDisplayInfoProvider); + // this forces lazy evaluation of the suggestion metadata + AutoCompleteHelper.ConvertDeclarationsToCompletionItems(suggestions, 1, 8, 8); - // this forces lazy evaluation of the suggestion metadata - AutoCompleteHelper.ConvertDeclarationsToCompletionItems(suggestions, 1, 8, 8); + parseResult = Parser.Parse( + "exec ", + bindingContext.ParseOptions); - parseResult = Parser.Parse( - "exec ", - bindingContext.ParseOptions); + parseResults = new List(); + parseResults.Add(parseResult); + bindingContext.Binder.Bind( + parseResults, + info.ConnectionDetails.DatabaseName, + BindMode.Batch); - parseResults = new List(); - parseResults.Add(parseResult); - bindingContext.Binder.Bind( - parseResults, - info.ConnectionDetails.DatabaseName, - BindMode.Batch); + // get the completion list from SQL Parser + suggestions = Resolver.FindCompletions( + parseResult, 1, 6, + bindingContext.MetadataDisplayInfoProvider); - // get the completion list from SQL Parser - suggestions = Resolver.FindCompletions( - parseResult, 1, 6, - bindingContext.MetadataDisplayInfoProvider); - - // this forces lazy evaluation of the suggestion metadata - AutoCompleteHelper.ConvertDeclarationsToCompletionItems(suggestions, 1, 6, 6); + // this forces lazy evaluation of the suggestion metadata + AutoCompleteHelper.ConvertDeclarationsToCompletionItems(suggestions, 1, 6, 6); + } return null; }); @@ -1103,6 +1107,16 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices Message = SR.PeekDefinitionTimedoutError, Locations = null }; + }, + errorHandler: ex => + { + // return error result + return new DefinitionResult + { + IsErrorResult = true, + Message = ex.Message, + Locations = null + }; }); // wait for the queue item diff --git a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/QueueItem.cs b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/QueueItem.cs index 2b45de8b..30cc3548 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/QueueItem.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/LanguageServices/QueueItem.cs @@ -36,6 +36,13 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices /// public Func TimeoutOperation { get; set; } + /// + /// Gets or sets the operation to call if the bind operation encounters an unexpected exception. + /// Supports returning an object in case of the exception occurring since in some cases we need to be + /// tolerant of error cases and still return some value + /// + public Func ErrorHandler { get; set; } + /// /// Gets or sets an event to signal when this queue item has been processed /// diff --git a/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.cs b/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.cs index 6cfb1c95..a3fdb6bf 100755 --- a/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.cs @@ -77,6 +77,30 @@ namespace Microsoft.SqlTools.ServiceLayer } } + public static string AzureSqlDbEdition + { + get + { + return Keys.GetString(Keys.AzureSqlDbEdition); + } + } + + public static string AzureSqlDwEdition + { + get + { + return Keys.GetString(Keys.AzureSqlDwEdition); + } + } + + public static string AzureSqlStretchEdition + { + get + { + return Keys.GetString(Keys.AzureSqlStretchEdition); + } + } + public static string QueryServiceCancelAlreadyCompleted { get @@ -3652,6 +3676,15 @@ namespace Microsoft.SqlTools.ServiceLayer public const string ConnectionParamsValidateNullSqlAuth = "ConnectionParamsValidateNullSqlAuth"; + public const string AzureSqlDbEdition = "AzureSqlDbEdition"; + + + public const string AzureSqlDwEdition = "AzureSqlDwEdition"; + + + public const string AzureSqlStretchEdition = "AzureSqlStretchEdition"; + + public const string QueryServiceCancelAlreadyCompleted = "QueryServiceCancelAlreadyCompleted"; diff --git a/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.resx b/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.resx index d83ce345..88515160 100755 --- a/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.resx +++ b/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.resx @@ -166,6 +166,18 @@ . Parameters: 0 - component (string) + + Azure SQL DB + + + + Azure SQL Data Warehouse + + + + Azure SQL Stretch Database + + The query has already completed, it cannot be cancelled diff --git a/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.strings b/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.strings index c1e00cda..fe050287 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.strings +++ b/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.strings @@ -47,6 +47,11 @@ ConnectionParamsValidateNullServerName = ServerName cannot be null or empty ConnectionParamsValidateNullSqlAuth(string component) = {0} cannot be null or empty when using SqlLogin authentication +### General connection service strings +AzureSqlDbEdition = Azure SQL DB +AzureSqlDwEdition = Azure SQL Data Warehouse +AzureSqlStretchEdition = Azure SQL Stretch Database + ############################################################################ # Query Execution Service diff --git a/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.xlf b/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.xlf index 8c452ae5..5d8d3424 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.xlf +++ b/src/Microsoft.SqlTools.ServiceLayer/Localization/sr.xlf @@ -2280,6 +2280,21 @@ Never + + Azure SQL DB + Azure SQL DB + + + + Azure SQL Data Warehouse + Azure SQL Data Warehouse + + + + Azure SQL Stretch Database + Azure SQL Stretch Database + + \ No newline at end of file diff --git a/src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingService.cs b/src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingService.cs index d24aa9b0..c173df87 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingService.cs @@ -238,7 +238,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Scripting string script = string.Empty; ScriptingObject scriptingObject = parameters.ScriptingObjects[0]; try - { + { Server server = new Server(bindingContext.ServerConnection); server.DefaultTextMode = true; @@ -277,7 +277,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Scripting } return null; - }); + }); } /// diff --git a/src/Microsoft.SqlTools.ServiceLayer/sr.es.resx b/src/Microsoft.SqlTools.ServiceLayer/sr.es.resx deleted file mode 100644 index b1657f70..00000000 --- a/src/Microsoft.SqlTools.ServiceLayer/sr.es.resx +++ /dev/null @@ -1,123 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - text/microsoft-resx - - - 2.0 - - - System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - - - System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - - - ES_LOCALIZATION - - \ No newline at end of file diff --git a/test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/PeekDefinitionTests.cs b/test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/PeekDefinitionTests.cs index d32a51b8..9bf09da7 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/PeekDefinitionTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/PeekDefinitionTests.cs @@ -211,6 +211,7 @@ GO"; It.IsAny(), It.IsAny>(), It.IsAny>(), + It.IsAny>(), It.IsAny(), It.IsAny())) .Callback, Func, int?, int?>( diff --git a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/BindingQueueTests.cs b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/BindingQueueTests.cs index 3b1653c0..06d03641 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/BindingQueueTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/BindingQueueTests.cs @@ -3,6 +3,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. // +using System; using System.Threading; using Microsoft.SqlServer.Management.Common; using Microsoft.SqlServer.Management.SmoMetadataProvider; @@ -133,6 +134,35 @@ namespace Microsoft.SqlTools.ServiceLayer.UnitTests.LanguageServer Assert.False(this.isCancelationRequested); } + /// + /// Queues a single task + /// + [Fact] + public void QueueWithUnhandledExceptionTest() + { + InitializeTestSettings(); + ManualResetEvent mre = new ManualResetEvent(false); + bool isExceptionHandled = false; + object defaultReturnObject = new object(); + var queueItem = this.bindingQueue.QueueBindingOperation( + key: "testkey", + bindOperation: (context, CancellationToken) => throw new Exception("Unhandled!!"), + timeoutOperation: TestTimeoutOperation, + errorHandler: (exception) => { + isExceptionHandled = true; + mre.Set(); + return defaultReturnObject; + }); + + mre.WaitOne(10000); + + this.bindingQueue.StopQueueProcessor(15000); + + Assert.True(isExceptionHandled); + var result = queueItem.GetResultAsT(); + Assert.Equal(defaultReturnObject, result); + } + /// /// Queue a 100 short tasks ///