maxmustermann/PowerToys
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix for Crash due to System.AccessViolationException when calling OleDBSearch from multiple threads. (#6052)

Browse source 
* Removing non thread safe member variables, as well as the check to ensure that they have been disposed.

* Removing 'ExecuteQuery_ShouldDisposeAllConnections_AfterFunctionCall'.

This call previously required the use of member variables that were not thread safe.   There is no reason to share this state across threads.  Arguably this test verifies an internal implementation detail and is not suitable for a unit test anyway.
This commit is contained in:
ryanbodrug-microsoft 2020-08-19 17:28:19 -07:00 committed by GitHub
parent 64106cba82
commit 2390368d03
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 12 additions and 101 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/modules/launcher/Plugins/Microsoft.Plugin.Indexer/Main.cs
View file

@ -253,7 +253,6 @@ namespace Microsoft.Plugin.Indexer
{
if (disposing)
{
_search.Dispose();
}
disposedValue = true;

96
src/modules/launcher/Plugins/Microsoft.Plugin.Indexer/SearchHelper/OleDBSearch.cs
View file

@ -8,13 +8,8 @@ using System.Data.OleDb;
namespace Microsoft.Plugin.Indexer.SearchHelper
{
public class OleDBSearch : ISearch, IDisposable
public class OleDBSearch : ISearch
{
private OleDbCommand command;
private OleDbConnection conn;
private OleDbDataReader wDSResults;
private bool disposedValue;
[System.Diagnostics.CodeAnalysis.SuppressMessage(
"Security",
"CA2100:Review SQL queries for security vulnerabilities",
@ -22,101 +17,34 @@ namespace Microsoft.Plugin.Indexer.SearchHelper
public List<OleDBResult> Query(string connectionString, string sqlQuery)
{
List<OleDBResult> result = new List<OleDBResult>();
using (conn = new OleDbConnection(connectionString))
using (var conn = new OleDbConnection(connectionString))
{
// open the connection
conn.Open();
try
// now create an OleDB command object with the query we built above and the connection we just opened.
using (var command = new OleDbCommand(sqlQuery, conn))
{
// now create an OleDB command object with the query we built above and the connection we just opened.
using (command = new OleDbCommand(sqlQuery, conn))
using (var wDSResults = command.ExecuteReader())
{
using (wDSResults = command.ExecuteReader())
if (!wDSResults.IsClosed && wDSResults.HasRows)
{
if (!wDSResults.IsClosed && wDSResults.HasRows)
while (wDSResults.IsClosed && wDSResults.Read())
{
while (!wDSResults.IsClosed && wDSResults.Read())
List<object> fieldData = new List<object>(wDSResults.FieldCount);
for (int i = 0; i < wDSResults.FieldCount; i++)
{
List<object> fieldData = new List<object>(wDSResults.FieldCount);
for (int i = 0; i < wDSResults.FieldCount; i++)
{
fieldData.Add(wDSResults.GetValue(i));
}
result.Add(new OleDBResult(fieldData));
fieldData.Add(wDSResults.GetValue(i));
}
result.Add(new OleDBResult(fieldData));
}
}
}
}
// AccessViolationException can occur if another query is made before the current query completes. Since the old query would be cancelled we can ignore the exception
catch (System.AccessViolationException)
{
// do nothing
}
}
return result;
}
// Checks if all the variables related to database connection have been properly disposed
public bool HaveAllDisposableItemsBeenDisposed()
{
bool commandDisposed = false;
bool connDisposed = false;
bool resultDisposed = false;
try
{
command.ExecuteReader();
}
catch (InvalidOperationException)
{
commandDisposed = true;
}
try
{
wDSResults.Read();
}
catch (InvalidOperationException)
{
resultDisposed = true;
}
if (conn.State == System.Data.ConnectionState.Closed)
{
connDisposed = true;
}
return commandDisposed && resultDisposed && connDisposed;
}
protected virtual void Dispose(bool disposing)
{
if (!disposedValue)
{
if (disposing)
{
command?.Dispose();
conn?.Dispose();
wDSResults?.Dispose();
}
// TODO: free unmanaged resources (unmanaged objects) and override finalizer
// TODO: set large fields to null
disposedValue = true;
}
}
public void Dispose()
{
// Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
Dispose(disposing: true);
GC.SuppressFinalize(this);
}
}
}

16
src/modules/launcher/Wox.Test/Plugins/WindowsIndexerTest.cs
View file

@ -164,22 +164,6 @@ namespace Wox.Test.Plugins
Assert.IsTrue(queryHelper.QueryWhereRestrictions.Contains("Contains"));
}
[Test]
[Ignore("This method is throwing the follwoing exception in CI. Ignoring temporarily until I understand why. Can't repro locally. System.Data.OleDb.OleDbException : IErrorInfo.GetDescription failed with E_FAIL(0x80004005).")]
public void ExecuteQuery_ShouldDisposeAllConnections_AfterFunctionCall()
{
// Arrange
OleDBSearch oleDbSearch = new OleDBSearch();
WindowsSearchAPI api = new WindowsSearchAPI(oleDbSearch);
var mockSearchManager = GetMockSearchManager();
// Act
api.Search("FilePath", mockSearchManager);
// Assert
Assert.IsTrue(oleDbSearch.HaveAllDisposableItemsBeenDisposed());
}
[Test]
public void WindowsSearchAPI_ShouldReturnResults_WhenSearchWasExecuted()
{