From 1b8e9c1e86685474212535ecc72edd9db5c8fdb7 Mon Sep 17 00:00:00 2001 From: Benjamin Russell Date: Mon, 3 Oct 2016 11:35:58 -0700 Subject: [PATCH] Lingering File Handles (#71) Fixing a bug where in various situations, the files used for temporary storage of query results would be leftover. In particular, the following changes were made: * When the dispose query request is submitted, the corresponding query is now disposed in addition from being removed from the list of active queries * When a query is cancelled, it is disposed after it is cancelled * If a query already exists for a given ownerURI, the existing query is disposed before creating a new query * All queries are disposed when the query execution service is disposed (ie, at shutdown of the service) A unit test to verify the action of the dispose method for a ResultSet was added. * Ensuring queries are disposed Adding logic to dispose any queries when: * URI that already has a query executes another query * A request to dispose a query is submitted * A request to cancel a query is submitted * Small tweaks for cleanup of query execution service --- .../QueryExecution/QueryExecutionService.cs | 25 +++++++++- .../QueryExecution/DisposeTests.cs | 49 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs index d8222972..d0ff8d1a 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs @@ -13,6 +13,7 @@ using Microsoft.SqlTools.ServiceLayer.Hosting.Protocol; using Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts; using Microsoft.SqlTools.ServiceLayer.QueryExecution.DataStorage; using Microsoft.SqlTools.ServiceLayer.SqlContext; +using Microsoft.SqlTools.ServiceLayer.Utility; using Microsoft.SqlTools.ServiceLayer.Workspace; using Microsoft.SqlTools.ServiceLayer.Workspace.Contracts; using Newtonsoft.Json; @@ -207,6 +208,9 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution return; } + // Cleanup the query + result.Dispose(); + // Success await requestContext.SendResult(new QueryDisposeResult { @@ -237,6 +241,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution // Cancel the query result.Cancel(); + result.Dispose(); // Attempt to dispose the query if (!ActiveQueries.TryRemove(cancelParams.OwnerUri, out result)) @@ -268,7 +273,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution /// /// Process request to save a resultSet to a file in CSV format /// - public async Task HandleSaveResultsAsCsvRequest( SaveResultsAsCsvRequestParams saveParams, + public async Task HandleSaveResultsAsCsvRequest(SaveResultsAsCsvRequestParams saveParams, RequestContext requestContext) { // retrieve query for OwnerUri @@ -341,7 +346,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution /// /// Process request to save a resultSet to a file in JSON format /// - public async Task HandleSaveResultsAsJsonRequest( SaveResultsAsJsonRequestParams saveParams, + public async Task HandleSaveResultsAsJsonRequest(SaveResultsAsJsonRequestParams saveParams, RequestContext requestContext) { // retrieve query for OwnerUri @@ -422,6 +427,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution await requestContext.SendError(ex.Message); } } + #endregion #region Private Helpers @@ -445,6 +451,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution Query oldQuery; if (ActiveQueries.TryGetValue(executeParams.OwnerUri, out oldQuery) && oldQuery.HasExecuted) { + oldQuery.Dispose(); ActiveQueries.TryRemove(executeParams.OwnerUri, out oldQuery); } @@ -546,8 +553,22 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution { foreach (var query in ActiveQueries) { + if (!query.Value.HasExecuted) + { + try + { + query.Value.Cancel(); + } + catch (Exception e) + { + // We don't particularly care if we fail to cancel during shutdown + string message = string.Format("Failed to cancel query {0} during query service disposal: {1}", query.Key, e); + Logger.Write(LogLevel.Warning, message); + } + } query.Value.Dispose(); } + ActiveQueries.Clear(); } disposed = true; diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/DisposeTests.cs b/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/DisposeTests.cs index 226afcc8..2837e892 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/DisposeTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/DisposeTests.cs @@ -4,9 +4,12 @@ // using System; +using System.Data.Common; using System.Threading.Tasks; using Microsoft.SqlTools.ServiceLayer.Hosting.Protocol; +using Microsoft.SqlTools.ServiceLayer.QueryExecution; using Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts; +using Microsoft.SqlTools.ServiceLayer.QueryExecution.DataStorage; using Microsoft.SqlTools.ServiceLayer.SqlContext; using Microsoft.SqlTools.ServiceLayer.Test.Utility; using Microsoft.SqlTools.ServiceLayer.Workspace; @@ -18,6 +21,21 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution { public class DisposeTests { + [Fact] + public void DisposeResultSet() + { + // Setup: Mock file stream factory, mock db reader + var mockFileStreamFactory = new Mock(); + var mockDataReader = Common.CreateTestConnection(null, false).CreateCommand().ExecuteReaderAsync().Result; + + // If: I setup a single resultset and then dispose it + ResultSet rs = new ResultSet(mockDataReader, mockFileStreamFactory.Object); + rs.Dispose(); + + // Then: The file that was created should have been deleted + mockFileStreamFactory.Verify(fsf => fsf.DisposeFile(It.IsAny()), Times.Once); + } + [Fact] public void DisposeExecutedQuery() { @@ -70,6 +88,37 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution Assert.NotEmpty(result.Messages); } + [Fact] + public async Task ServiceDispose() + { + // Setup: + // ... We need a workspace service that returns a file + var fileMock = new Mock(); + fileMock.SetupGet(file => file.Contents).Returns(Common.StandardQuery); + var workspaceService = new Mock>(); + workspaceService.Setup(service => service.Workspace.GetFile(It.IsAny())) + .Returns(fileMock.Object); + // ... We need a query service + var queryService = Common.GetPrimedExecutionService(Common.CreateMockFactory(null, false), true, + workspaceService.Object); + + // If: + // ... I execute some bogus query + var queryParams = new QueryExecuteParams { QuerySelection = Common.WholeDocument, OwnerUri = Common.OwnerUri }; + var requestContext = RequestContextMocks.Create(null); + await queryService.HandleExecuteRequest(queryParams, requestContext.Object); + + // ... And it sticks around as an active query + Assert.Equal(1, queryService.ActiveQueries.Count); + + // ... The query execution service is disposed, like when the service is shutdown + queryService.Dispose(); + + // Then: + // ... There should no longer be an active query + Assert.Empty(queryService.ActiveQueries); + } + #region Mocking private Mock> GetQueryDisposeResultContextMock(