From 309ee82644875268529f3a93421f0513150954e1 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Tue, 14 Apr 2020 18:20:52 -0700 Subject: [PATCH 01/11] Use safe handles for SQLite interop --- .../Shared/Extensions/SafeHandleExtensions.cs | 44 +++++++ .../Shared/Extensions/SafeHandleLease.cs | 26 ++++ .../SQLite/v1/Interop/SafeSqliteBlobHandle.cs | 40 ++++++ .../SQLite/v1/Interop/SafeSqliteHandle.cs | 35 ++++++ .../v1/Interop/SafeSqliteStatementHandle.cs | 40 ++++++ .../SQLite/v1/Interop/SqlConnection.cs | 117 +++++++++++------- .../Storage/SQLite/v1/Interop/SqlStatement.cs | 59 +++++++-- 7 files changed, 303 insertions(+), 58 deletions(-) create mode 100644 src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleExtensions.cs create mode 100644 src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleLease.cs create mode 100644 src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs create mode 100644 src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs create mode 100644 src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs diff --git a/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleExtensions.cs b/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleExtensions.cs new file mode 100644 index 0000000000000..aec0a28aa1e74 --- /dev/null +++ b/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleExtensions.cs @@ -0,0 +1,44 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#nullable enable + +using System; +using System.Runtime.InteropServices; +using Roslyn.Utilities; + +namespace Microsoft.CodeAnalysis.Shared.Extensions +{ + internal static class SafeHandleExtensions + { + /// + /// Acquires a lease on a safe handle. This method is intended to be used in the initializer of a using + /// statement. + /// + /// The to lease. + /// A , which must be disposed to release the resource. + /// If the lease could not be acquired. + public static SafeHandleLease Lease(this SafeHandle handle) + { + RoslynDebug.AssertNotNull(handle); + + var success = false; + try + { + handle.DangerousAddRef(ref success); + if (!success) + throw new ObjectDisposedException(handle.GetType().FullName); + + return new SafeHandleLease(handle); + } + catch + { + if (success) + handle.DangerousRelease(); + + throw; + } + } + } +} diff --git a/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleLease.cs b/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleLease.cs new file mode 100644 index 0000000000000..05bad8e2594a0 --- /dev/null +++ b/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleLease.cs @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#nullable enable + +using System; +using System.Runtime.InteropServices; + +namespace Microsoft.CodeAnalysis.Shared.Extensions +{ + /// + /// Represents a lease of a . + /// + /// + internal readonly struct SafeHandleLease : IDisposable + { + private readonly SafeHandle _handle; + + internal SafeHandleLease(SafeHandle handle) + => _handle = handle; + + public void Dispose() + => _handle?.DangerousRelease(); + } +} diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs new file mode 100644 index 0000000000000..63da9d5729829 --- /dev/null +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs @@ -0,0 +1,40 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#nullable enable + +using System; +using System.Runtime.InteropServices; +using Microsoft.CodeAnalysis.Shared.Extensions; +using SQLitePCL; + +namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +{ + internal sealed class SafeSqliteBlobHandle : SafeHandle + { + private readonly SafeHandleLease _lease; + private readonly sqlite3_blob _wrapper; + + public SafeSqliteBlobHandle(SafeSqliteHandle handle, sqlite3_blob blob) + : base(IntPtr.Zero, ownsHandle: true) + { + _lease = handle.Lease(); + _wrapper = blob; + SetHandle(blob.ptr); + } + + public override bool IsInvalid => handle == IntPtr.Zero; + + public new sqlite3_blob DangerousGetHandle() + => _wrapper; + + protected override bool ReleaseHandle() + { + using var _ = _lease; + + raw.sqlite3_blob_close(_wrapper); + return true; + } + } +} diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs new file mode 100644 index 0000000000000..ed5bf6ab899ce --- /dev/null +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs @@ -0,0 +1,35 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#nullable enable + +using System; +using System.Runtime.InteropServices; +using SQLitePCL; + +namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +{ + internal sealed class SafeSqliteHandle : SafeHandle + { + private readonly sqlite3 _wrapper; + + public SafeSqliteHandle(sqlite3 handle) + : base(IntPtr.Zero, ownsHandle: true) + { + _wrapper = handle; + SetHandle(handle.ptr); + } + + public override bool IsInvalid => handle == IntPtr.Zero; + + public new sqlite3 DangerousGetHandle() + => _wrapper; + + protected override bool ReleaseHandle() + { + raw.sqlite3_close(_wrapper); + return true; + } + } +} diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs new file mode 100644 index 0000000000000..44fbb7df1df34 --- /dev/null +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs @@ -0,0 +1,40 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#nullable enable + +using System; +using System.Runtime.InteropServices; +using Microsoft.CodeAnalysis.Shared.Extensions; +using SQLitePCL; + +namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +{ + internal sealed class SafeSqliteStatementHandle : SafeHandle + { + private readonly SafeHandleLease _lease; + private readonly sqlite3_stmt _wrapper; + + public SafeSqliteStatementHandle(SafeSqliteHandle handle, sqlite3_stmt rawStatement) + : base(IntPtr.Zero, ownsHandle: true) + { + _lease = handle.Lease(); + _wrapper = rawStatement; + SetHandle(rawStatement.ptr); + } + + public override bool IsInvalid => handle == IntPtr.Zero; + + public new sqlite3_stmt DangerousGetHandle() + => _wrapper; + + protected override bool ReleaseHandle() + { + using var _ = _lease; + + raw.sqlite3_finalize(_wrapper); + return true; + } + } +} diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs index 026c59a51f1f7..1076cd5133f94 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs @@ -5,8 +5,8 @@ using System; using System.Collections.Generic; using System.IO; -using Microsoft.CodeAnalysis.ErrorReporting; using Microsoft.CodeAnalysis.Host; +using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; using SQLitePCL; @@ -30,7 +30,7 @@ internal class SqlConnection /// /// The raw handle to the underlying DB. /// - private readonly sqlite3 _handle; + private readonly SafeSqliteHandle _handle; /// /// For testing purposes to simulate failures during testing. @@ -70,44 +70,41 @@ public static SqlConnection Create(IPersistentStorageFaultInjector faultInjector Contract.ThrowIfNull(handle); + SafeSqliteHandle wrapper; try { - raw.sqlite3_busy_timeout(handle, (int)TimeSpan.FromMinutes(1).TotalMilliseconds); - return new SqlConnection(handle, faultInjector, queryToStatement); + wrapper = new SafeSqliteHandle(handle); + } + catch + { + raw.sqlite3_close(handle); + throw; + } + + try + { + using var _ = wrapper.Lease(); + raw.sqlite3_busy_timeout(wrapper.DangerousGetHandle(), (int)TimeSpan.FromMinutes(1).TotalMilliseconds); + return new SqlConnection(wrapper, faultInjector, queryToStatement); } catch { // If we failed to create connection, ensure that we still release the sqlite // handle. - raw.sqlite3_close(handle); + wrapper.Dispose(); throw; } } - private SqlConnection(sqlite3 handle, IPersistentStorageFaultInjector faultInjector, Dictionary queryToStatement) + private SqlConnection(SafeSqliteHandle handle, IPersistentStorageFaultInjector faultInjector, Dictionary queryToStatement) { - // This constructor avoids allocations since failure (e.g. OutOfMemoryException) would - // leave the object partially-constructed, and the finalizer would run later triggering - // a crash. _handle = handle; _faultInjector = faultInjector; _queryToStatement = queryToStatement; } - ~SqlConnection() - { - if (!Environment.HasShutdownStarted) - { - var ex = new InvalidOperationException("SqlConnection was not properly closed"); - _faultInjector?.OnFatalError(ex); - FatalError.Report(new InvalidOperationException("SqlConnection was not properly closed")); - } - } - internal void Close_OnlyForUseBySqlPersistentStorage() { - GC.SuppressFinalize(this); - Contract.ThrowIfNull(_handle); // release all the cached statements we have. @@ -123,7 +120,7 @@ internal void Close_OnlyForUseBySqlPersistentStorage() _queryToStatement.Clear(); // Finally close our handle to the actual DB. - ThrowIfNotOk(raw.sqlite3_close(_handle)); + _handle.Dispose(); } public void ExecuteCommand(string command, bool throwOnError = true) @@ -141,10 +138,32 @@ public ResettableSqlStatement GetResettableStatement(string query) { if (!_queryToStatement.TryGetValue(query, out var statement)) { - var result = (Result)raw.sqlite3_prepare_v2(_handle, query, out var rawStatement); + using var _ = _handle.Lease(); + + var result = (Result)raw.sqlite3_prepare_v2(_handle.DangerousGetHandle(), query, out var rawStatement); ThrowIfNotOk(result); - statement = new SqlStatement(this, rawStatement); - _queryToStatement[query] = statement; + + SafeSqliteStatementHandle wrapper; + try + { + wrapper = new SafeSqliteStatementHandle(_handle, rawStatement); + } + catch + { + raw.sqlite3_finalize(rawStatement); + throw; + } + + try + { + statement = new SqlStatement(this, wrapper); + _queryToStatement[query] = statement; + } + catch + { + wrapper.Dispose(); + throw; + } } return new ResettableSqlStatement(statement); @@ -216,7 +235,10 @@ private void Rollback(bool throwOnError) => ExecuteCommand("rollback transaction", throwOnError); public int LastInsertRowId() - => (int)raw.sqlite3_last_insert_rowid(_handle); + { + using var _ = _handle.Lease(); + return (int)raw.sqlite3_last_insert_rowid(_handle.DangerousGetHandle()); + } [PerformanceSensitive("https://github.com/dotnet/roslyn/issues/36114", AllowCaptures = false)] public Stream ReadBlob_MustRunInTransaction(string tableName, string columnName, long rowId) @@ -236,7 +258,9 @@ public Stream ReadBlob_MustRunInTransaction(string tableName, string columnName, } const int ReadOnlyFlags = 0; - var result = raw.sqlite3_blob_open(_handle, "main", tableName, columnName, rowId, ReadOnlyFlags, out var blob); + + using var _ = _handle.Lease(); + var result = raw.sqlite3_blob_open(_handle.DangerousGetHandle(), "main", tableName, columnName, rowId, ReadOnlyFlags, out var blob); if (result == raw.SQLITE_ERROR) { // can happen when rowId points to a row that hasn't been written to yet. @@ -244,19 +268,16 @@ public Stream ReadBlob_MustRunInTransaction(string tableName, string columnName, } ThrowIfNotOk(result); - try - { - return ReadBlob(blob); - } - finally - { - ThrowIfNotOk(raw.sqlite3_blob_close(blob)); - } + + using var blobHandle = new SafeSqliteBlobHandle(_handle, blob); + return ReadBlob(blobHandle); } - private Stream ReadBlob(sqlite3_blob blob) + private Stream ReadBlob(SafeSqliteBlobHandle blob) { - var length = raw.sqlite3_blob_bytes(blob); + using var _ = blob.Lease(); + + var length = raw.sqlite3_blob_bytes(blob.DangerousGetHandle()); // If it's a small blob, just read it into one of our pooled arrays, and then // create a PooledStream over it. @@ -268,17 +289,19 @@ private Stream ReadBlob(sqlite3_blob blob) { // Otherwise, it's a large stream. Just take the hit of allocating. var bytes = new byte[length]; - ThrowIfNotOk(raw.sqlite3_blob_read(blob, bytes, length, offset: 0)); + ThrowIfNotOk(raw.sqlite3_blob_read(blob.DangerousGetHandle(), bytes, length, offset: 0)); return new MemoryStream(bytes); } } - private Stream ReadBlobIntoPooledStream(sqlite3_blob blob, int length) + private Stream ReadBlobIntoPooledStream(SafeSqliteBlobHandle blob, int length) { + using var _ = blob.Lease(); + var bytes = SQLitePersistentStorage.GetPooledBytes(); try { - ThrowIfNotOk(raw.sqlite3_blob_read(blob, bytes, length, offset: 0)); + ThrowIfNotOk(raw.sqlite3_blob_read(blob.DangerousGetHandle(), bytes, length, offset: 0)); // Copy those bytes into a pooled stream return SerializableBytes.CreateReadableStream(bytes, length); @@ -296,7 +319,7 @@ public void ThrowIfNotOk(int result) public void ThrowIfNotOk(Result result) => ThrowIfNotOk(_handle, result); - public static void ThrowIfNotOk(sqlite3 handle, Result result) + public static void ThrowIfNotOk(SafeSqliteHandle handle, Result result) { if (result != Result.OK) { @@ -307,9 +330,13 @@ public static void ThrowIfNotOk(sqlite3 handle, Result result) public void Throw(Result result) => Throw(_handle, result); - public static void Throw(sqlite3 handle, Result result) - => throw new SqlException(result, - raw.sqlite3_errmsg(handle) + "\r\n" + - raw.sqlite3_errstr(raw.sqlite3_extended_errcode(handle))); + public static void Throw(SafeSqliteHandle handle, Result result) + { + using var _ = handle.Lease(); + + throw new SqlException(result, + raw.sqlite3_errmsg(handle.DangerousGetHandle()) + "\r\n" + + raw.sqlite3_errstr(raw.sqlite3_extended_errcode(handle.DangerousGetHandle()))); + } } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs index 6429d2bb44af3..88e483c46e981 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs @@ -4,6 +4,7 @@ using System; using System.Runtime.InteropServices; +using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; using SQLitePCL; @@ -30,23 +31,31 @@ namespace Microsoft.CodeAnalysis.SQLite.v1.Interop internal struct SqlStatement { private readonly SqlConnection _connection; - private readonly sqlite3_stmt _rawStatement; + private readonly SafeSqliteStatementHandle _rawStatement; - public SqlStatement(SqlConnection connection, sqlite3_stmt statement) + public SqlStatement(SqlConnection connection, SafeSqliteStatementHandle statement) { _connection = connection; _rawStatement = statement; } internal void Close_OnlyForUseBySqlConnection() - => _connection.ThrowIfNotOk(raw.sqlite3_finalize(_rawStatement)); + { + _rawStatement.Dispose(); + } public void Reset() - => _connection.ThrowIfNotOk(raw.sqlite3_reset(_rawStatement)); + { + using var _ = _rawStatement.Lease(); + + _connection.ThrowIfNotOk(raw.sqlite3_reset(_rawStatement.DangerousGetHandle())); + } public Result Step(bool throwOnError = true) { - var stepResult = (Result)raw.sqlite3_step(_rawStatement); + using var _ = _rawStatement.Lease(); + + var stepResult = (Result)raw.sqlite3_step(_rawStatement.DangerousGetHandle()); // Anything other than DONE or ROW is an error when stepping. // throw if the caller wants that, or just return the value @@ -64,30 +73,54 @@ public Result Step(bool throwOnError = true) } internal void BindStringParameter(int parameterIndex, string value) - => _connection.ThrowIfNotOk(raw.sqlite3_bind_text(_rawStatement, parameterIndex, value)); + { + using var _ = _rawStatement.Lease(); + + _connection.ThrowIfNotOk(raw.sqlite3_bind_text(_rawStatement.DangerousGetHandle(), parameterIndex, value)); + } internal void BindInt64Parameter(int parameterIndex, long value) - => _connection.ThrowIfNotOk(raw.sqlite3_bind_int64(_rawStatement, parameterIndex, value)); + { + using var _ = _rawStatement.Lease(); + + _connection.ThrowIfNotOk(raw.sqlite3_bind_int64(_rawStatement.DangerousGetHandle(), parameterIndex, value)); + } // SQLite PCL does not expose sqlite3_bind_blob function that takes a length. So we explicitly // DLL import it here. See https://github.com/ericsink/SQLitePCL.raw/issues/135 internal void BindBlobParameter(int parameterIndex, byte[] value, int length) - => _connection.ThrowIfNotOk(sqlite3_bind_blob(_rawStatement.ptr, parameterIndex, value, length, new IntPtr(-1))); + => _connection.ThrowIfNotOk(sqlite3_bind_blob(_rawStatement, parameterIndex, value, length, new IntPtr(-1))); [DllImport("e_sqlite3.dll", ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)] - public static extern int sqlite3_bind_blob(IntPtr stmt, int index, byte[] val, int nSize, IntPtr nTransient); + public static extern int sqlite3_bind_blob(SafeSqliteStatementHandle stmt, int index, byte[] val, int nSize, IntPtr nTransient); internal byte[] GetBlobAt(int columnIndex) - => raw.sqlite3_column_blob(_rawStatement, columnIndex); + { + using var _ = _rawStatement.Lease(); + + return raw.sqlite3_column_blob(_rawStatement.DangerousGetHandle(), columnIndex); + } internal int GetInt32At(int columnIndex) - => raw.sqlite3_column_int(_rawStatement, columnIndex); + { + using var _ = _rawStatement.Lease(); + + return raw.sqlite3_column_int(_rawStatement.DangerousGetHandle(), columnIndex); + } internal long GetInt64At(int columnIndex) - => raw.sqlite3_column_int64(_rawStatement, columnIndex); + { + using var _ = _rawStatement.Lease(); + + return raw.sqlite3_column_int64(_rawStatement.DangerousGetHandle(), columnIndex); + } internal string GetStringAt(int columnIndex) - => raw.sqlite3_column_text(_rawStatement, columnIndex); + { + using var _ = _rawStatement.Lease(); + + return raw.sqlite3_column_text(_rawStatement.DangerousGetHandle(), columnIndex); + } } } From e60863193396695dd70110414fe528f288cf60a6 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 15 Apr 2020 09:08:59 -0700 Subject: [PATCH 02/11] Simplify SQLite safe handles --- .../Shared/Extensions/SafeHandleExtensions.cs | 9 +++-- .../Shared/Extensions/SafeHandleLease.cs | 6 +++- .../SQLite/v1/Interop/SafeSqliteBlobHandle.cs | 26 +++----------- .../v1/Interop/SafeSqliteChildHandle`1.cs | 34 +++++++++++++++++++ .../SQLite/v1/Interop/SafeSqliteHandle.cs | 19 +++-------- .../SQLite/v1/Interop/SafeSqliteHandle`1.cs | 28 +++++++++++++++ .../v1/Interop/SafeSqliteStatementHandle.cs | 26 +++----------- 7 files changed, 88 insertions(+), 60 deletions(-) create mode 100644 src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs create mode 100644 src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs diff --git a/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleExtensions.cs b/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleExtensions.cs index aec0a28aa1e74..7869519621f02 100644 --- a/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleExtensions.cs +++ b/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleExtensions.cs @@ -13,9 +13,14 @@ namespace Microsoft.CodeAnalysis.Shared.Extensions internal static class SafeHandleExtensions { /// - /// Acquires a lease on a safe handle. This method is intended to be used in the initializer of a using - /// statement. + /// Acquires a lease on a safe handle. The lease increments the reference count of the + /// to ensure the handle is not released prior to the lease being released. /// + /// + /// This method is intended to be used in the initializer of a using statement. Failing to release the + /// lease will permanently prevent the underlying from being released by the garbage + /// collector. + /// /// The to lease. /// A , which must be disposed to release the resource. /// If the lease could not be acquired. diff --git a/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleLease.cs b/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleLease.cs index 05bad8e2594a0..901fb2d3ec73d 100644 --- a/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleLease.cs +++ b/src/Workspaces/Core/Portable/Shared/Extensions/SafeHandleLease.cs @@ -15,11 +15,15 @@ namespace Microsoft.CodeAnalysis.Shared.Extensions /// internal readonly struct SafeHandleLease : IDisposable { - private readonly SafeHandle _handle; + private readonly SafeHandle? _handle; internal SafeHandleLease(SafeHandle handle) => _handle = handle; + /// + /// Releases the lease. The behavior of this method is unspecified if called more than + /// once. + /// public void Dispose() => _handle?.DangerousRelease(); } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs index 63da9d5729829..10900e683da79 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs @@ -4,36 +4,20 @@ #nullable enable -using System; -using System.Runtime.InteropServices; -using Microsoft.CodeAnalysis.Shared.Extensions; using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { - internal sealed class SafeSqliteBlobHandle : SafeHandle + internal sealed class SafeSqliteBlobHandle : SafeSqliteChildHandle { - private readonly SafeHandleLease _lease; - private readonly sqlite3_blob _wrapper; - - public SafeSqliteBlobHandle(SafeSqliteHandle handle, sqlite3_blob blob) - : base(IntPtr.Zero, ownsHandle: true) + public SafeSqliteBlobHandle(SafeSqliteHandle sqliteHandle, sqlite3_blob wrapper) + : base(sqliteHandle, wrapper.ptr, wrapper) { - _lease = handle.Lease(); - _wrapper = blob; - SetHandle(blob.ptr); } - public override bool IsInvalid => handle == IntPtr.Zero; - - public new sqlite3_blob DangerousGetHandle() - => _wrapper; - - protected override bool ReleaseHandle() + protected override bool ReleaseChildHandle() { - using var _ = _lease; - - raw.sqlite3_blob_close(_wrapper); + raw.sqlite3_blob_close(Wrapper); return true; } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs new file mode 100644 index 0000000000000..499e4d82ddf20 --- /dev/null +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs @@ -0,0 +1,34 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.InteropServices; +using Microsoft.CodeAnalysis.Shared.Extensions; + +namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +{ + /// + /// The base handle type for an SQLite resource that exists within the context of a parent handle, and should always + /// be released prior to the parent handle. + /// + /// The SQLite resource wrapper type. + internal abstract class SafeSqliteChildHandle : SafeSqliteHandle + { + private readonly SafeHandleLease _lease; + + protected SafeSqliteChildHandle(SafeHandle parentHandle, IntPtr handle, T wrapper) + : base(handle, wrapper) + { + _lease = parentHandle.Lease(); + } + + protected abstract bool ReleaseChildHandle(); + + protected sealed override bool ReleaseHandle() + { + using var _ = _lease; + return ReleaseChildHandle(); + } + } +} diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs index ed5bf6ab899ce..9f3eaa5ab86c0 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs @@ -4,31 +4,20 @@ #nullable enable -using System; -using System.Runtime.InteropServices; using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { - internal sealed class SafeSqliteHandle : SafeHandle + internal sealed class SafeSqliteHandle : SafeSqliteHandle { - private readonly sqlite3 _wrapper; - - public SafeSqliteHandle(sqlite3 handle) - : base(IntPtr.Zero, ownsHandle: true) + public SafeSqliteHandle(sqlite3 wrapper) + : base(wrapper.ptr, wrapper) { - _wrapper = handle; - SetHandle(handle.ptr); } - public override bool IsInvalid => handle == IntPtr.Zero; - - public new sqlite3 DangerousGetHandle() - => _wrapper; - protected override bool ReleaseHandle() { - raw.sqlite3_close(_wrapper); + raw.sqlite3_close(Wrapper); return true; } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs new file mode 100644 index 0000000000000..1672a78cf8ecd --- /dev/null +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs @@ -0,0 +1,28 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#nullable enable + +using System; +using System.Runtime.InteropServices; + +namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +{ + internal abstract class SafeSqliteHandle : SafeHandle + { + protected readonly T Wrapper; + + public SafeSqliteHandle(IntPtr handle, T wrapper) + : base(invalidHandleValue: IntPtr.Zero, ownsHandle: true) + { + Wrapper = wrapper; + SetHandle(handle); + } + + public override bool IsInvalid => handle == IntPtr.Zero; + + public new T DangerousGetHandle() + => Wrapper; + } +} diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs index 44fbb7df1df34..595ccc3b50076 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs @@ -4,36 +4,20 @@ #nullable enable -using System; -using System.Runtime.InteropServices; -using Microsoft.CodeAnalysis.Shared.Extensions; using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { - internal sealed class SafeSqliteStatementHandle : SafeHandle + internal sealed class SafeSqliteStatementHandle : SafeSqliteChildHandle { - private readonly SafeHandleLease _lease; - private readonly sqlite3_stmt _wrapper; - - public SafeSqliteStatementHandle(SafeSqliteHandle handle, sqlite3_stmt rawStatement) - : base(IntPtr.Zero, ownsHandle: true) + public SafeSqliteStatementHandle(SafeSqliteHandle sqliteHandle, sqlite3_stmt wrapper) + : base(sqliteHandle, wrapper.ptr, wrapper) { - _lease = handle.Lease(); - _wrapper = rawStatement; - SetHandle(rawStatement.ptr); } - public override bool IsInvalid => handle == IntPtr.Zero; - - public new sqlite3_stmt DangerousGetHandle() - => _wrapper; - - protected override bool ReleaseHandle() + protected override bool ReleaseChildHandle() { - using var _ = _lease; - - raw.sqlite3_finalize(_wrapper); + raw.sqlite3_finalize(Wrapper); return true; } } From 763a4c3f5774ef44fe0de93e83e236fc884ba4fc Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 15 Apr 2020 10:05:46 -0700 Subject: [PATCH 03/11] Refactor code to simplify handle creation --- .../Portable/Storage/SQLite/NativeMethods.cs | 80 +++++++++++++++++++ .../Storage/SQLite/v1/Interop/Result.cs | 2 +- .../SQLite/v1/Interop/SafeSqliteBlobHandle.cs | 5 +- .../v1/Interop/SafeSqliteChildHandle`1.cs | 5 +- .../SQLite/v1/Interop/SafeSqliteHandle.cs | 5 +- .../SQLite/v1/Interop/SafeSqliteHandle`1.cs | 7 +- .../v1/Interop/SafeSqliteStatementHandle.cs | 5 +- .../SQLite/v1/Interop/SqlConnection.cs | 52 ++++-------- 8 files changed, 112 insertions(+), 49 deletions(-) create mode 100644 src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs b/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs new file mode 100644 index 0000000000000..0cb056ed0d993 --- /dev/null +++ b/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs @@ -0,0 +1,80 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#nullable enable + +using System.Diagnostics.CodeAnalysis; +using Microsoft.CodeAnalysis.Shared.Extensions; +using Microsoft.CodeAnalysis.SQLite.v1.Interop; +using SQLitePCL; + +namespace Microsoft.CodeAnalysis.SQLite +{ + internal static class NativeMethods + { + [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Name chosen to match SQLitePCL.raw")] + public static SafeSqliteHandle sqlite3_open_v2(string filename, int flags, string vfs, out Result result) + { + result = (Result)raw.sqlite3_open_v2(filename, out var wrapper, flags, vfs); + if (result != Result.OK) + { + wrapper = null; + } + + try + { + return new SafeSqliteHandle(wrapper); + } + catch + { + raw.sqlite3_close(wrapper); + throw; + } + } + + [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Name chosen to match SQLitePCL.raw")] + public static SafeSqliteStatementHandle sqlite3_prepare_v2(SafeSqliteHandle db, string sql, out Result result) + { + using var _ = db.Lease(); + + result = (Result)raw.sqlite3_prepare_v2(db.DangerousGetHandle(), sql, out var wrapper); + if (result != (int)Result.OK) + { + wrapper = null; + } + + try + { + return new SafeSqliteStatementHandle(db, wrapper); + } + catch + { + raw.sqlite3_finalize(wrapper); + throw; + } + } + + [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Name chosen to match SQLitePCL.raw")] + public static SafeSqliteBlobHandle sqlite3_blob_open(SafeSqliteHandle db, string sdb, string table, string col, long rowid, int flags, out Result result) + { + using var _ = db.Lease(); + + result = (Result)raw.sqlite3_blob_open(db.DangerousGetHandle(), sdb, table, col, rowid, flags, out var wrapper); + if (result != (int)Result.OK) + { + wrapper = null; + } + + try + { + return new SafeSqliteBlobHandle(db, wrapper); + } + catch + { + raw.sqlite3_blob_close(wrapper); + throw; + } + } + } +} diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/Result.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/Result.cs index 61acc328dacb2..33536fd44bfde 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/Result.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/Result.cs @@ -10,7 +10,7 @@ namespace Microsoft.CodeAnalysis.SQLite.v1.Interop internal enum Result { OK = 0, /* Successful result */ - // ERROR = 1, /* SQL error or missing database */ + ERROR = 1, /* SQL error or missing database */ // INTERNAL = 2, /* Internal logic error in SQLite */ // PERM = 3, /* Access permission denied */ // ABORT = 4, /* Callback routine requested an abort */ diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs index 10900e683da79..c450dd859febc 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs @@ -4,14 +4,15 @@ #nullable enable +using System; using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { internal sealed class SafeSqliteBlobHandle : SafeSqliteChildHandle { - public SafeSqliteBlobHandle(SafeSqliteHandle sqliteHandle, sqlite3_blob wrapper) - : base(sqliteHandle, wrapper.ptr, wrapper) + public SafeSqliteBlobHandle(SafeSqliteHandle sqliteHandle, sqlite3_blob? wrapper) + : base(sqliteHandle, wrapper?.ptr ?? IntPtr.Zero, wrapper) { } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs index 499e4d82ddf20..5e5692bd34d5f 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs @@ -2,6 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +#nullable enable + using System; using System.Runtime.InteropServices; using Microsoft.CodeAnalysis.Shared.Extensions; @@ -14,10 +16,11 @@ namespace Microsoft.CodeAnalysis.SQLite.v1.Interop /// /// The SQLite resource wrapper type. internal abstract class SafeSqliteChildHandle : SafeSqliteHandle + where T : class { private readonly SafeHandleLease _lease; - protected SafeSqliteChildHandle(SafeHandle parentHandle, IntPtr handle, T wrapper) + protected SafeSqliteChildHandle(SafeHandle parentHandle, IntPtr handle, T? wrapper) : base(handle, wrapper) { _lease = parentHandle.Lease(); diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs index 9f3eaa5ab86c0..512fd3a3d5483 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs @@ -4,14 +4,15 @@ #nullable enable +using System; using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { internal sealed class SafeSqliteHandle : SafeSqliteHandle { - public SafeSqliteHandle(sqlite3 wrapper) - : base(wrapper.ptr, wrapper) + public SafeSqliteHandle(sqlite3? wrapper) + : base(wrapper?.ptr ?? IntPtr.Zero, wrapper) { } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs index 1672a78cf8ecd..6087b81bda536 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs @@ -10,10 +10,11 @@ namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { internal abstract class SafeSqliteHandle : SafeHandle + where T : class { - protected readonly T Wrapper; + protected readonly T? Wrapper; - public SafeSqliteHandle(IntPtr handle, T wrapper) + public SafeSqliteHandle(IntPtr handle, T? wrapper) : base(invalidHandleValue: IntPtr.Zero, ownsHandle: true) { Wrapper = wrapper; @@ -23,6 +24,6 @@ public SafeSqliteHandle(IntPtr handle, T wrapper) public override bool IsInvalid => handle == IntPtr.Zero; public new T DangerousGetHandle() - => Wrapper; + => Wrapper!; } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs index 595ccc3b50076..54131c6791826 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs @@ -4,14 +4,15 @@ #nullable enable +using System; using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { internal sealed class SafeSqliteStatementHandle : SafeSqliteChildHandle { - public SafeSqliteStatementHandle(SafeSqliteHandle sqliteHandle, sqlite3_stmt wrapper) - : base(sqliteHandle, wrapper.ptr, wrapper) + public SafeSqliteStatementHandle(SafeSqliteHandle sqliteHandle, sqlite3_stmt? wrapper) + : base(sqliteHandle, wrapper?.ptr ?? IntPtr.Zero, wrapper) { } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs index 1076cd5133f94..3751ed9ca3c36 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs @@ -61,37 +61,25 @@ public static SqlConnection Create(IPersistentStorageFaultInjector faultInjector // one is only used from a single thread at a time. // see https://sqlite.org/threadsafe.html for more detail var flags = OpenFlags.SQLITE_OPEN_CREATE | OpenFlags.SQLITE_OPEN_READWRITE | OpenFlags.SQLITE_OPEN_NOMUTEX; - var result = (Result)raw.sqlite3_open_v2(databasePath, out var handle, (int)flags, vfs: null); + var handle = NativeMethods.sqlite3_open_v2(databasePath, (int)flags, vfs: null, out var result); if (result != Result.OK) { + handle.Dispose(); throw new SqlException(result, $"Could not open database file: {databasePath} ({result})"); } - Contract.ThrowIfNull(handle); - - SafeSqliteHandle wrapper; - try - { - wrapper = new SafeSqliteHandle(handle); - } - catch - { - raw.sqlite3_close(handle); - throw; - } - try { - using var _ = wrapper.Lease(); - raw.sqlite3_busy_timeout(wrapper.DangerousGetHandle(), (int)TimeSpan.FromMinutes(1).TotalMilliseconds); - return new SqlConnection(wrapper, faultInjector, queryToStatement); + using var _ = handle.Lease(); + raw.sqlite3_busy_timeout(handle.DangerousGetHandle(), (int)TimeSpan.FromMinutes(1).TotalMilliseconds); + return new SqlConnection(handle, faultInjector, queryToStatement); } catch { // If we failed to create connection, ensure that we still release the sqlite // handle. - wrapper.Dispose(); + handle.Dispose(); throw; } } @@ -140,28 +128,17 @@ public ResettableSqlStatement GetResettableStatement(string query) { using var _ = _handle.Lease(); - var result = (Result)raw.sqlite3_prepare_v2(_handle.DangerousGetHandle(), query, out var rawStatement); - ThrowIfNotOk(result); - - SafeSqliteStatementHandle wrapper; + var handle = NativeMethods.sqlite3_prepare_v2(_handle, query, out var result); try { - wrapper = new SafeSqliteStatementHandle(_handle, rawStatement); - } - catch - { - raw.sqlite3_finalize(rawStatement); - throw; - } + ThrowIfNotOk(result); - try - { - statement = new SqlStatement(this, wrapper); + statement = new SqlStatement(this, handle); _queryToStatement[query] = statement; } catch { - wrapper.Dispose(); + handle.Dispose(); throw; } } @@ -259,9 +236,9 @@ public Stream ReadBlob_MustRunInTransaction(string tableName, string columnName, const int ReadOnlyFlags = 0; - using var _ = _handle.Lease(); - var result = raw.sqlite3_blob_open(_handle.DangerousGetHandle(), "main", tableName, columnName, rowId, ReadOnlyFlags, out var blob); - if (result == raw.SQLITE_ERROR) + using var blob = NativeMethods.sqlite3_blob_open(_handle, "main", tableName, columnName, rowId, ReadOnlyFlags, out var result); + + if (result == Result.ERROR) { // can happen when rowId points to a row that hasn't been written to yet. return null; @@ -269,8 +246,7 @@ public Stream ReadBlob_MustRunInTransaction(string tableName, string columnName, ThrowIfNotOk(result); - using var blobHandle = new SafeSqliteBlobHandle(_handle, blob); - return ReadBlob(blobHandle); + return ReadBlob(blob); } private Stream ReadBlob(SafeSqliteBlobHandle blob) From 95ad938d99e16b19c24c2d1f6be4739aff5431bf Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 15 Apr 2020 10:25:14 -0700 Subject: [PATCH 04/11] Consolidate use of DangerousGetHandle to NativeMethods --- .../Portable/Storage/SQLite/NativeMethods.cs | 88 ++++++++++++++++++- .../SQLite/v1/Interop/SqlConnection.cs | 27 ++---- .../Storage/SQLite/v1/Interop/SqlStatement.cs | 48 ++-------- 3 files changed, 100 insertions(+), 63 deletions(-) diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs b/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs index 0cb056ed0d993..5e95dd871dfa2 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs @@ -11,9 +11,9 @@ namespace Microsoft.CodeAnalysis.SQLite { + [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Name chosen to match SQLitePCL.raw")] internal static class NativeMethods { - [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Name chosen to match SQLitePCL.raw")] public static SafeSqliteHandle sqlite3_open_v2(string filename, int flags, string vfs, out Result result) { result = (Result)raw.sqlite3_open_v2(filename, out var wrapper, flags, vfs); @@ -33,7 +33,6 @@ public static SafeSqliteHandle sqlite3_open_v2(string filename, int flags, strin } } - [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Name chosen to match SQLitePCL.raw")] public static SafeSqliteStatementHandle sqlite3_prepare_v2(SafeSqliteHandle db, string sql, out Result result) { using var _ = db.Lease(); @@ -55,7 +54,6 @@ public static SafeSqliteStatementHandle sqlite3_prepare_v2(SafeSqliteHandle db, } } - [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Name chosen to match SQLitePCL.raw")] public static SafeSqliteBlobHandle sqlite3_blob_open(SafeSqliteHandle db, string sdb, string table, string col, long rowid, int flags, out Result result) { using var _ = db.Lease(); @@ -76,5 +74,89 @@ public static SafeSqliteBlobHandle sqlite3_blob_open(SafeSqliteHandle db, string throw; } } + + public static string sqlite3_errmsg(SafeSqliteHandle db) + { + using var _ = db.Lease(); + return raw.sqlite3_errmsg(db.DangerousGetHandle()); + } + + public static int sqlite3_extended_errcode(SafeSqliteHandle db) + { + using var _ = db.Lease(); + return raw.sqlite3_extended_errcode(db.DangerousGetHandle()); + } + + public static Result sqlite3_busy_timeout(SafeSqliteHandle db, int ms) + { + using var _ = db.Lease(); + return (Result)raw.sqlite3_busy_timeout(db.DangerousGetHandle(), ms); + } + + public static long sqlite3_last_insert_rowid(SafeSqliteHandle db) + { + using var _ = db.Lease(); + return raw.sqlite3_last_insert_rowid(db.DangerousGetHandle()); + } + + public static int sqlite3_blob_bytes(SafeSqliteBlobHandle blob) + { + using var _ = blob.Lease(); + return raw.sqlite3_blob_bytes(blob.DangerousGetHandle()); + } + + public static Result sqlite3_blob_read(SafeSqliteBlobHandle blob, byte[] b, int n, int offset) + { + using var _ = blob.Lease(); + return (Result)raw.sqlite3_blob_read(blob.DangerousGetHandle(), b, n, offset); + } + + public static Result sqlite3_reset(SafeSqliteStatementHandle stmt) + { + using var _ = stmt.Lease(); + return (Result)raw.sqlite3_reset(stmt.DangerousGetHandle()); + } + + public static Result sqlite3_step(SafeSqliteStatementHandle stmt) + { + using var _ = stmt.Lease(); + return (Result)raw.sqlite3_step(stmt.DangerousGetHandle()); + } + + public static Result sqlite3_bind_text(SafeSqliteStatementHandle stmt, int index, string val) + { + using var _ = stmt.Lease(); + return (Result)raw.sqlite3_bind_text(stmt.DangerousGetHandle(), index, val); + } + + public static Result sqlite3_bind_int64(SafeSqliteStatementHandle stmt, int index, long val) + { + using var _ = stmt.Lease(); + return (Result)raw.sqlite3_bind_int64(stmt.DangerousGetHandle(), index, val); + } + + public static byte[] sqlite3_column_blob(SafeSqliteStatementHandle stmt, int index) + { + using var _ = stmt.Lease(); + return raw.sqlite3_column_blob(stmt.DangerousGetHandle(), index); + } + + public static int sqlite3_column_int(SafeSqliteStatementHandle stmt, int index) + { + using var _ = stmt.Lease(); + return raw.sqlite3_column_int(stmt.DangerousGetHandle(), index); + } + + public static long sqlite3_column_int64(SafeSqliteStatementHandle stmt, int index) + { + using var _ = stmt.Lease(); + return raw.sqlite3_column_int64(stmt.DangerousGetHandle(), index); + } + + public static string sqlite3_column_text(SafeSqliteStatementHandle stmt, int index) + { + using var _ = stmt.Lease(); + return raw.sqlite3_column_text(stmt.DangerousGetHandle(), index); + } } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs index 3751ed9ca3c36..77231c427741b 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs @@ -6,7 +6,6 @@ using System.Collections.Generic; using System.IO; using Microsoft.CodeAnalysis.Host; -using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; using SQLitePCL; @@ -71,8 +70,7 @@ public static SqlConnection Create(IPersistentStorageFaultInjector faultInjector try { - using var _ = handle.Lease(); - raw.sqlite3_busy_timeout(handle.DangerousGetHandle(), (int)TimeSpan.FromMinutes(1).TotalMilliseconds); + NativeMethods.sqlite3_busy_timeout(handle, (int)TimeSpan.FromMinutes(1).TotalMilliseconds); return new SqlConnection(handle, faultInjector, queryToStatement); } catch @@ -126,8 +124,6 @@ public ResettableSqlStatement GetResettableStatement(string query) { if (!_queryToStatement.TryGetValue(query, out var statement)) { - using var _ = _handle.Lease(); - var handle = NativeMethods.sqlite3_prepare_v2(_handle, query, out var result); try { @@ -212,10 +208,7 @@ private void Rollback(bool throwOnError) => ExecuteCommand("rollback transaction", throwOnError); public int LastInsertRowId() - { - using var _ = _handle.Lease(); - return (int)raw.sqlite3_last_insert_rowid(_handle.DangerousGetHandle()); - } + => (int)NativeMethods.sqlite3_last_insert_rowid(_handle); [PerformanceSensitive("https://github.com/dotnet/roslyn/issues/36114", AllowCaptures = false)] public Stream ReadBlob_MustRunInTransaction(string tableName, string columnName, long rowId) @@ -251,9 +244,7 @@ public Stream ReadBlob_MustRunInTransaction(string tableName, string columnName, private Stream ReadBlob(SafeSqliteBlobHandle blob) { - using var _ = blob.Lease(); - - var length = raw.sqlite3_blob_bytes(blob.DangerousGetHandle()); + var length = NativeMethods.sqlite3_blob_bytes(blob); // If it's a small blob, just read it into one of our pooled arrays, and then // create a PooledStream over it. @@ -265,19 +256,17 @@ private Stream ReadBlob(SafeSqliteBlobHandle blob) { // Otherwise, it's a large stream. Just take the hit of allocating. var bytes = new byte[length]; - ThrowIfNotOk(raw.sqlite3_blob_read(blob.DangerousGetHandle(), bytes, length, offset: 0)); + ThrowIfNotOk(NativeMethods.sqlite3_blob_read(blob, bytes, length, offset: 0)); return new MemoryStream(bytes); } } private Stream ReadBlobIntoPooledStream(SafeSqliteBlobHandle blob, int length) { - using var _ = blob.Lease(); - var bytes = SQLitePersistentStorage.GetPooledBytes(); try { - ThrowIfNotOk(raw.sqlite3_blob_read(blob.DangerousGetHandle(), bytes, length, offset: 0)); + ThrowIfNotOk(NativeMethods.sqlite3_blob_read(blob, bytes, length, offset: 0)); // Copy those bytes into a pooled stream return SerializableBytes.CreateReadableStream(bytes, length); @@ -308,11 +297,9 @@ public void Throw(Result result) public static void Throw(SafeSqliteHandle handle, Result result) { - using var _ = handle.Lease(); - throw new SqlException(result, - raw.sqlite3_errmsg(handle.DangerousGetHandle()) + "\r\n" + - raw.sqlite3_errstr(raw.sqlite3_extended_errcode(handle.DangerousGetHandle()))); + NativeMethods.sqlite3_errmsg(handle) + "\r\n" + + raw.sqlite3_errstr(NativeMethods.sqlite3_extended_errcode(handle))); } } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs index 88e483c46e981..cd8dc75133fc6 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs @@ -4,9 +4,7 @@ using System; using System.Runtime.InteropServices; -using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; -using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { @@ -45,17 +43,11 @@ internal void Close_OnlyForUseBySqlConnection() } public void Reset() - { - using var _ = _rawStatement.Lease(); - - _connection.ThrowIfNotOk(raw.sqlite3_reset(_rawStatement.DangerousGetHandle())); - } + => _connection.ThrowIfNotOk(NativeMethods.sqlite3_reset(_rawStatement)); public Result Step(bool throwOnError = true) { - using var _ = _rawStatement.Lease(); - - var stepResult = (Result)raw.sqlite3_step(_rawStatement.DangerousGetHandle()); + var stepResult = NativeMethods.sqlite3_step(_rawStatement); // Anything other than DONE or ROW is an error when stepping. // throw if the caller wants that, or just return the value @@ -73,18 +65,10 @@ public Result Step(bool throwOnError = true) } internal void BindStringParameter(int parameterIndex, string value) - { - using var _ = _rawStatement.Lease(); - - _connection.ThrowIfNotOk(raw.sqlite3_bind_text(_rawStatement.DangerousGetHandle(), parameterIndex, value)); - } + => _connection.ThrowIfNotOk(NativeMethods.sqlite3_bind_text(_rawStatement, parameterIndex, value)); internal void BindInt64Parameter(int parameterIndex, long value) - { - using var _ = _rawStatement.Lease(); - - _connection.ThrowIfNotOk(raw.sqlite3_bind_int64(_rawStatement.DangerousGetHandle(), parameterIndex, value)); - } + => _connection.ThrowIfNotOk(NativeMethods.sqlite3_bind_int64(_rawStatement, parameterIndex, value)); // SQLite PCL does not expose sqlite3_bind_blob function that takes a length. So we explicitly // DLL import it here. See https://github.com/ericsink/SQLitePCL.raw/issues/135 @@ -96,31 +80,15 @@ internal void BindBlobParameter(int parameterIndex, byte[] value, int length) public static extern int sqlite3_bind_blob(SafeSqliteStatementHandle stmt, int index, byte[] val, int nSize, IntPtr nTransient); internal byte[] GetBlobAt(int columnIndex) - { - using var _ = _rawStatement.Lease(); - - return raw.sqlite3_column_blob(_rawStatement.DangerousGetHandle(), columnIndex); - } + => NativeMethods.sqlite3_column_blob(_rawStatement, columnIndex); internal int GetInt32At(int columnIndex) - { - using var _ = _rawStatement.Lease(); - - return raw.sqlite3_column_int(_rawStatement.DangerousGetHandle(), columnIndex); - } + => NativeMethods.sqlite3_column_int(_rawStatement, columnIndex); internal long GetInt64At(int columnIndex) - { - using var _ = _rawStatement.Lease(); - - return raw.sqlite3_column_int64(_rawStatement.DangerousGetHandle(), columnIndex); - } + => NativeMethods.sqlite3_column_int64(_rawStatement, columnIndex); internal string GetStringAt(int columnIndex) - { - using var _ = _rawStatement.Lease(); - - return raw.sqlite3_column_text(_rawStatement.DangerousGetHandle(), columnIndex); - } + => NativeMethods.sqlite3_column_text(_rawStatement, columnIndex); } } From 39c31e3b1bc262000ed9d1f35b99c8b45c13f1b7 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 16 Apr 2020 10:25:12 -0700 Subject: [PATCH 05/11] Update ReleaseHandle to check return values --- .../Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs | 4 ++-- .../Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs | 4 ++-- .../Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs index c450dd859febc..2f5bff1e7e5ea 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs @@ -18,8 +18,8 @@ public SafeSqliteBlobHandle(SafeSqliteHandle sqliteHandle, sqlite3_blob? wrapper protected override bool ReleaseChildHandle() { - raw.sqlite3_blob_close(Wrapper); - return true; + var result = (Result)raw.sqlite3_blob_close(Wrapper); + return result == Result.OK; } } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs index 512fd3a3d5483..476f590aaa5da 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs @@ -18,8 +18,8 @@ public SafeSqliteHandle(sqlite3? wrapper) protected override bool ReleaseHandle() { - raw.sqlite3_close(Wrapper); - return true; + var result = (Result)raw.sqlite3_close(Wrapper); + return result == Result.OK; } } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs index 54131c6791826..c974c2c02be1d 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs @@ -18,8 +18,8 @@ public SafeSqliteStatementHandle(SafeSqliteHandle sqliteHandle, sqlite3_stmt? wr protected override bool ReleaseChildHandle() { - raw.sqlite3_finalize(Wrapper); - return true; + var result = (Result)raw.sqlite3_finalize(Wrapper); + return result == Result.OK; } } } From de40b567a507b1a2c7fe5a36d33444e059c9b89b Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 16 Apr 2020 10:25:32 -0700 Subject: [PATCH 06/11] Document non-null return values --- .../Core/Portable/Storage/SQLite/NativeMethods.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs b/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs index 5e95dd871dfa2..aff6924dc951e 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs @@ -24,6 +24,8 @@ public static SafeSqliteHandle sqlite3_open_v2(string filename, int flags, strin try { + // Always return a non-null handle to match default P/Invoke marshaling behavior. SafeHandle.IsInvalid + // will be true when the handle is not usable, but the handle instance can be disposed either way. return new SafeSqliteHandle(wrapper); } catch @@ -45,6 +47,8 @@ public static SafeSqliteStatementHandle sqlite3_prepare_v2(SafeSqliteHandle db, try { + // Always return a non-null handle to match default P/Invoke marshaling behavior. SafeHandle.IsInvalid + // will be true when the handle is not usable, but the handle instance can be disposed either way. return new SafeSqliteStatementHandle(db, wrapper); } catch @@ -66,6 +70,8 @@ public static SafeSqliteBlobHandle sqlite3_blob_open(SafeSqliteHandle db, string try { + // Always return a non-null handle to match default P/Invoke marshaling behavior. SafeHandle.IsInvalid + // will be true when the handle is not usable, but the handle instance can be disposed either way. return new SafeSqliteBlobHandle(db, wrapper); } catch From 771d6c9668b5115bd5de74456a57d80afb900631 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 16 Apr 2020 10:32:50 -0700 Subject: [PATCH 07/11] Simplify the release of the database handle --- .../Portable/Storage/SQLite/v1/Interop/SqlConnection.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs index 77231c427741b..2f058c046426c 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs @@ -91,7 +91,8 @@ private SqlConnection(SafeSqliteHandle handle, IPersistentStorageFaultInjector f internal void Close_OnlyForUseBySqlPersistentStorage() { - Contract.ThrowIfNull(_handle); + // Dispose of the underlying handle at the end of cleanup + using var _ = _handle; // release all the cached statements we have. // @@ -104,9 +105,6 @@ internal void Close_OnlyForUseBySqlPersistentStorage() } _queryToStatement.Clear(); - - // Finally close our handle to the actual DB. - _handle.Dispose(); } public void ExecuteCommand(string command, bool throwOnError = true) From 03aa9f11e9fd15e1cb59df551dedd3ae06704b2d Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 16 Apr 2020 11:19:27 -0700 Subject: [PATCH 08/11] Move SQLite safe handles to a common interop namespace --- .../Portable/Storage/SQLite/{ => Interop}/NativeMethods.cs | 3 +-- .../Core/Portable/Storage/SQLite/{v1 => }/Interop/Result.cs | 2 +- .../Storage/SQLite/{v1 => }/Interop/SafeSqliteBlobHandle.cs | 2 +- .../Storage/SQLite/{v1 => }/Interop/SafeSqliteChildHandle`1.cs | 2 +- .../Storage/SQLite/{v1 => }/Interop/SafeSqliteHandle.cs | 2 +- .../Storage/SQLite/{v1 => }/Interop/SafeSqliteHandle`1.cs | 2 +- .../SQLite/{v1 => }/Interop/SafeSqliteStatementHandle.cs | 2 +- .../Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs | 1 + .../Core/Portable/Storage/SQLite/v1/Interop/SqlException.cs | 1 + .../Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs | 1 + .../Storage/SQLite/v1/SQLitePersistentStorage.Accessor.cs | 1 + .../SQLite/v1/SQLitePersistentStorage_BulkPopulateIds.cs | 1 + .../Storage/SQLite/v1/SQLitePersistentStorage_StringIds.cs | 1 + 13 files changed, 13 insertions(+), 8 deletions(-) rename src/Workspaces/Core/Portable/Storage/SQLite/{ => Interop}/NativeMethods.cs (98%) rename src/Workspaces/Core/Portable/Storage/SQLite/{v1 => }/Interop/Result.cs (98%) rename src/Workspaces/Core/Portable/Storage/SQLite/{v1 => }/Interop/SafeSqliteBlobHandle.cs (93%) rename src/Workspaces/Core/Portable/Storage/SQLite/{v1 => }/Interop/SafeSqliteChildHandle`1.cs (95%) rename src/Workspaces/Core/Portable/Storage/SQLite/{v1 => }/Interop/SafeSqliteHandle.cs (92%) rename src/Workspaces/Core/Portable/Storage/SQLite/{v1 => }/Interop/SafeSqliteHandle`1.cs (93%) rename src/Workspaces/Core/Portable/Storage/SQLite/{v1 => }/Interop/SafeSqliteStatementHandle.cs (93%) diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs similarity index 98% rename from src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs rename to src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs index aff6924dc951e..f5323e2d10f41 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/NativeMethods.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs @@ -6,10 +6,9 @@ using System.Diagnostics.CodeAnalysis; using Microsoft.CodeAnalysis.Shared.Extensions; -using Microsoft.CodeAnalysis.SQLite.v1.Interop; using SQLitePCL; -namespace Microsoft.CodeAnalysis.SQLite +namespace Microsoft.CodeAnalysis.SQLite.Interop { [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Name chosen to match SQLitePCL.raw")] internal static class NativeMethods diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/Result.cs b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/Result.cs similarity index 98% rename from src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/Result.cs rename to src/Workspaces/Core/Portable/Storage/SQLite/Interop/Result.cs index 33536fd44bfde..3e09236ab30cc 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/Result.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/Result.cs @@ -2,7 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +namespace Microsoft.CodeAnalysis.SQLite.Interop { // From https://sqlite.org/c3ref/c_abort.html // Uncomment what you need. Leave the rest commented out to make it clear diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteBlobHandle.cs similarity index 93% rename from src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs rename to src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteBlobHandle.cs index 2f5bff1e7e5ea..f7625834711f6 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteBlobHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteBlobHandle.cs @@ -7,7 +7,7 @@ using System; using SQLitePCL; -namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +namespace Microsoft.CodeAnalysis.SQLite.Interop { internal sealed class SafeSqliteBlobHandle : SafeSqliteChildHandle { diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteChildHandle`1.cs similarity index 95% rename from src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs rename to src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteChildHandle`1.cs index 5e5692bd34d5f..40a1c7e77265e 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteChildHandle`1.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteChildHandle`1.cs @@ -8,7 +8,7 @@ using System.Runtime.InteropServices; using Microsoft.CodeAnalysis.Shared.Extensions; -namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +namespace Microsoft.CodeAnalysis.SQLite.Interop { /// /// The base handle type for an SQLite resource that exists within the context of a parent handle, and should always diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteHandle.cs similarity index 92% rename from src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs rename to src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteHandle.cs index 476f590aaa5da..2e66c42857b40 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteHandle.cs @@ -7,7 +7,7 @@ using System; using SQLitePCL; -namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +namespace Microsoft.CodeAnalysis.SQLite.Interop { internal sealed class SafeSqliteHandle : SafeSqliteHandle { diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteHandle`1.cs similarity index 93% rename from src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs rename to src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteHandle`1.cs index 6087b81bda536..77d7462545fab 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteHandle`1.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteHandle`1.cs @@ -7,7 +7,7 @@ using System; using System.Runtime.InteropServices; -namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +namespace Microsoft.CodeAnalysis.SQLite.Interop { internal abstract class SafeSqliteHandle : SafeHandle where T : class diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteStatementHandle.cs similarity index 93% rename from src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs rename to src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteStatementHandle.cs index c974c2c02be1d..bea28c3d24420 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SafeSqliteStatementHandle.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteStatementHandle.cs @@ -7,7 +7,7 @@ using System; using SQLitePCL; -namespace Microsoft.CodeAnalysis.SQLite.v1.Interop +namespace Microsoft.CodeAnalysis.SQLite.Interop { internal sealed class SafeSqliteStatementHandle : SafeSqliteChildHandle { diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs index 2f058c046426c..1cfc55379b935 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.IO; using Microsoft.CodeAnalysis.Host; +using Microsoft.CodeAnalysis.SQLite.Interop; using Roslyn.Utilities; using SQLitePCL; diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlException.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlException.cs index 4e2b9188a89a4..8988ecb6663d1 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlException.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlException.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using Microsoft.CodeAnalysis.SQLite.Interop; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs index cd8dc75133fc6..f56c54f3e5d84 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlStatement.cs @@ -4,6 +4,7 @@ using System; using System.Runtime.InteropServices; +using Microsoft.CodeAnalysis.SQLite.Interop; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage.Accessor.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage.Accessor.cs index 5d4fa2aee8e6f..cfa95ca52e28c 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage.Accessor.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage.Accessor.cs @@ -9,6 +9,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.PooledObjects; +using Microsoft.CodeAnalysis.SQLite.Interop; using Microsoft.CodeAnalysis.SQLite.v1.Interop; using Microsoft.CodeAnalysis.Storage; using Roslyn.Utilities; diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage_BulkPopulateIds.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage_BulkPopulateIds.cs index 13e9969714ceb..c12bb18a861fd 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage_BulkPopulateIds.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage_BulkPopulateIds.cs @@ -6,6 +6,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using Microsoft.CodeAnalysis.PooledObjects; +using Microsoft.CodeAnalysis.SQLite.Interop; using Microsoft.CodeAnalysis.SQLite.v1.Interop; using Microsoft.CodeAnalysis.Storage; diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage_StringIds.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage_StringIds.cs index 682748b97a8b9..896d50da4d010 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage_StringIds.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/SQLitePersistentStorage_StringIds.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; +using Microsoft.CodeAnalysis.SQLite.Interop; using Microsoft.CodeAnalysis.SQLite.v1.Interop; using Microsoft.CodeAnalysis.Storage; using Roslyn.Utilities; From e2ad0b0f8ec98bd29c02b283c71efadebd33ad64 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 16 Apr 2020 11:22:02 -0700 Subject: [PATCH 09/11] Avoid using SQLitePCL.raw outside NativeMethods --- .../Core/Portable/Storage/SQLite/Interop/NativeMethods.cs | 5 +++++ .../Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs | 3 +-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs index f5323e2d10f41..b6606916bf8e3 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs @@ -86,6 +86,11 @@ public static string sqlite3_errmsg(SafeSqliteHandle db) return raw.sqlite3_errmsg(db.DangerousGetHandle()); } + public static string sqlite3_errstr(int rc) + { + return raw.sqlite3_errstr(rc); + } + public static int sqlite3_extended_errcode(SafeSqliteHandle db) { using var _ = db.Lease(); diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs index 1cfc55379b935..825502b3faa36 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v1/Interop/SqlConnection.cs @@ -8,7 +8,6 @@ using Microsoft.CodeAnalysis.Host; using Microsoft.CodeAnalysis.SQLite.Interop; using Roslyn.Utilities; -using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v1.Interop { @@ -298,7 +297,7 @@ public static void Throw(SafeSqliteHandle handle, Result result) { throw new SqlException(result, NativeMethods.sqlite3_errmsg(handle) + "\r\n" + - raw.sqlite3_errstr(NativeMethods.sqlite3_extended_errcode(handle))); + NativeMethods.sqlite3_errstr(NativeMethods.sqlite3_extended_errcode(handle))); } } } From e648eee06fc2885b4eabb20994ff39d9a785dcf0 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 16 Apr 2020 11:34:56 -0700 Subject: [PATCH 10/11] Update the SQLite v2 implementation to use safe handles --- .../Storage/SQLite/v2/Interop/Result.cs | 44 --------- .../SQLite/v2/Interop/SqlConnection.cs | 89 ++++++++----------- .../Storage/SQLite/v2/Interop/SqlException.cs | 1 + .../Storage/SQLite/v2/Interop/SqlStatement.cs | 28 +++--- .../v2/SQLitePersistentStorage.Accessor.cs | 1 + ...SQLitePersistentStorage_BulkPopulateIds.cs | 1 + .../v2/SQLitePersistentStorage_StringIds.cs | 1 + 7 files changed, 55 insertions(+), 110 deletions(-) delete mode 100644 src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/Result.cs diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/Result.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/Result.cs deleted file mode 100644 index 312f7150d51c1..0000000000000 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/Result.cs +++ /dev/null @@ -1,44 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -namespace Microsoft.CodeAnalysis.SQLite.v2.Interop -{ - // From https://sqlite.org/c3ref/c_abort.html - // Uncomment what you need. Leave the rest commented out to make it clear - // what we are/aren't using. - internal enum Result - { - OK = 0, /* Successful result */ - // ERROR = 1, /* SQL error or missing database */ - // INTERNAL = 2, /* Internal logic error in SQLite */ - // PERM = 3, /* Access permission denied */ - // ABORT = 4, /* Callback routine requested an abort */ - BUSY = 5, /* The database file is locked */ - LOCKED = 6, /* A table in the database is locked */ - NOMEM = 7, /* A malloc() failed */ - // READONLY = 8, /* Attempt to write a readonly database */ - // INTERRUPT = 9, /* Operation terminated by sqlite3_interrupt()*/ - IOERR = 10, /* Some kind of disk I/O error occurred */ - // CORRUPT = 11, /* The database disk image is malformed */ - // NOTFOUND = 12, /* Unknown opcode in sqlite3_file_control() */ - FULL = 13, /* Insertion failed because database is full */ - // CANTOPEN = 14, /* Unable to open the database file */ - // PROTOCOL = 15, /* Database lock protocol error */ - // EMPTY = 16, /* Database is empty */ - // SCHEMA = 17, /* The database schema changed */ - // TOOBIG = 18, /* String or BLOB exceeds size limit */ - CONSTRAINT = 19, /* Abort due to constraint violation */ - // MISMATCH = 20, /* Data type mismatch */ - // MISUSE = 21, /* Library used incorrectly */ - // NOLFS = 22, /* Uses OS features not supported on host */ - // AUTH = 23, /* Authorization denied */ - // FORMAT = 24, /* Auxiliary database format error */ - // RANGE = 25, /* 2nd parameter to sqlite3_bind out of range */ - // NOTADB = 26, /* File opened that is not a database file */ - // NOTICE = 27, /* Notifications from sqlite3_log() */ - // WARNING = 28, /* Warnings from sqlite3_log() */ - ROW = 100, /* sqlite3_step() has another row ready */ - DONE = 101 /* sqlite3_step() has finished executing */ - } -} diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlConnection.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlConnection.cs index 0e809fa437978..4f77968f70cdc 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlConnection.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlConnection.cs @@ -5,10 +5,9 @@ using System; using System.Collections.Generic; using System.IO; -using Microsoft.CodeAnalysis.ErrorReporting; using Microsoft.CodeAnalysis.Host; +using Microsoft.CodeAnalysis.SQLite.Interop; using Roslyn.Utilities; -using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v2.Interop { @@ -30,7 +29,7 @@ internal class SqlConnection /// /// The raw handle to the underlying DB. /// - private readonly sqlite3 _handle; + private readonly SafeSqliteHandle _handle; /// /// For testing purposes to simulate failures during testing. @@ -70,18 +69,17 @@ public static SqlConnection Create(IPersistentStorageFaultInjector faultInjector OpenFlags.SQLITE_OPEN_NOMUTEX | OpenFlags.SQLITE_OPEN_SHAREDCACHE | OpenFlags.SQLITE_OPEN_URI; - var result = (Result)raw.sqlite3_open_v2(databasePath, out var handle, (int)flags, vfs: null); + var handle = NativeMethods.sqlite3_open_v2(databasePath, (int)flags, vfs: null, out var result); if (result != Result.OK) { + handle.Dispose(); throw new SqlException(result, $"Could not open database file: {databasePath} ({result})"); } - Contract.ThrowIfNull(handle); - try { - raw.sqlite3_busy_timeout(handle, (int)TimeSpan.FromMinutes(1).TotalMilliseconds); + NativeMethods.sqlite3_busy_timeout(handle, (int)TimeSpan.FromMinutes(1).TotalMilliseconds); var connection = new SqlConnection(handle, faultInjector, queryToStatement); // Attach (creating if necessary) a singleton in-memory write cache to this connection. @@ -105,36 +103,22 @@ public static SqlConnection Create(IPersistentStorageFaultInjector faultInjector { // If we failed to create connection, ensure that we still release the sqlite // handle. - raw.sqlite3_close(handle); + handle.Dispose(); throw; } } - private SqlConnection(sqlite3 handle, IPersistentStorageFaultInjector faultInjector, Dictionary queryToStatement) + private SqlConnection(SafeSqliteHandle handle, IPersistentStorageFaultInjector faultInjector, Dictionary queryToStatement) { - // This constructor avoids allocations since failure (e.g. OutOfMemoryException) would - // leave the object partially-constructed, and the finalizer would run later triggering - // a crash. _handle = handle; _faultInjector = faultInjector; _queryToStatement = queryToStatement; } - ~SqlConnection() - { - if (!Environment.HasShutdownStarted) - { - var ex = new InvalidOperationException("SqlConnection was not properly closed"); - _faultInjector?.OnFatalError(ex); - FatalError.Report(new InvalidOperationException("SqlConnection was not properly closed")); - } - } - internal void Close_OnlyForUseBySqlPersistentStorage() { - GC.SuppressFinalize(this); - - Contract.ThrowIfNull(_handle); + // Dispose of the underlying handle at the end of cleanup + using var _ = _handle; // release all the cached statements we have. // @@ -147,9 +131,6 @@ internal void Close_OnlyForUseBySqlPersistentStorage() } _queryToStatement.Clear(); - - // Finally close our handle to the actual DB. - ThrowIfNotOk(raw.sqlite3_close(_handle)); } public void ExecuteCommand(string command, bool throwOnError = true) @@ -167,10 +148,19 @@ public ResettableSqlStatement GetResettableStatement(string query) { if (!_queryToStatement.TryGetValue(query, out var statement)) { - var result = (Result)raw.sqlite3_prepare_v2(_handle, query, out var rawStatement); - ThrowIfNotOk(result); - statement = new SqlStatement(this, rawStatement); - _queryToStatement[query] = statement; + var handle = NativeMethods.sqlite3_prepare_v2(_handle, query, out var result); + try + { + ThrowIfNotOk(result); + + statement = new SqlStatement(this, handle); + _queryToStatement[query] = statement; + } + catch + { + handle.Dispose(); + throw; + } } return new ResettableSqlStatement(statement); @@ -242,7 +232,7 @@ private void Rollback(bool throwOnError) => ExecuteCommand("rollback transaction", throwOnError); public int LastInsertRowId() - => (int)raw.sqlite3_last_insert_rowid(_handle); + => (int)NativeMethods.sqlite3_last_insert_rowid(_handle); [PerformanceSensitive("https://github.com/dotnet/roslyn/issues/36114", AllowCaptures = false)] public Stream ReadBlob_MustRunInTransaction(Database database, string tableName, string columnName, long rowId) @@ -262,27 +252,22 @@ public Stream ReadBlob_MustRunInTransaction(Database database, string tableName, } const int ReadOnlyFlags = 0; - var result = raw.sqlite3_blob_open(_handle, database.GetName(), tableName, columnName, rowId, ReadOnlyFlags, out var blob); - if (result == raw.SQLITE_ERROR) + + using var blob = NativeMethods.sqlite3_blob_open(_handle, database.GetName(), tableName, columnName, rowId, ReadOnlyFlags, out var result); + + if (result == Result.ERROR) { // can happen when rowId points to a row that hasn't been written to yet. return null; } ThrowIfNotOk(result); - try - { - return ReadBlob(blob); - } - finally - { - ThrowIfNotOk(raw.sqlite3_blob_close(blob)); - } + return ReadBlob(blob); } - private Stream ReadBlob(sqlite3_blob blob) + private Stream ReadBlob(SafeSqliteBlobHandle blob) { - var length = raw.sqlite3_blob_bytes(blob); + var length = NativeMethods.sqlite3_blob_bytes(blob); // If it's a small blob, just read it into one of our pooled arrays, and then // create a PooledStream over it. @@ -294,17 +279,17 @@ private Stream ReadBlob(sqlite3_blob blob) { // Otherwise, it's a large stream. Just take the hit of allocating. var bytes = new byte[length]; - ThrowIfNotOk(raw.sqlite3_blob_read(blob, bytes, length, offset: 0)); + ThrowIfNotOk(NativeMethods.sqlite3_blob_read(blob, bytes, length, offset: 0)); return new MemoryStream(bytes); } } - private Stream ReadBlobIntoPooledStream(sqlite3_blob blob, int length) + private Stream ReadBlobIntoPooledStream(SafeSqliteBlobHandle blob, int length) { var bytes = SQLitePersistentStorage.GetPooledBytes(); try { - ThrowIfNotOk(raw.sqlite3_blob_read(blob, bytes, length, offset: 0)); + ThrowIfNotOk(NativeMethods.sqlite3_blob_read(blob, bytes, length, offset: 0)); // Copy those bytes into a pooled stream return SerializableBytes.CreateReadableStream(bytes, length); @@ -322,7 +307,7 @@ public void ThrowIfNotOk(int result) public void ThrowIfNotOk(Result result) => ThrowIfNotOk(_handle, result); - public static void ThrowIfNotOk(sqlite3 handle, Result result) + public static void ThrowIfNotOk(SafeSqliteHandle handle, Result result) { if (result != Result.OK) { @@ -333,9 +318,9 @@ public static void ThrowIfNotOk(sqlite3 handle, Result result) public void Throw(Result result) => Throw(_handle, result); - public static void Throw(sqlite3 handle, Result result) + public static void Throw(SafeSqliteHandle handle, Result result) => throw new SqlException(result, - raw.sqlite3_errmsg(handle) + "\r\n" + - raw.sqlite3_errstr(raw.sqlite3_extended_errcode(handle))); + NativeMethods.sqlite3_errmsg(handle) + "\r\n" + + NativeMethods.sqlite3_errstr(NativeMethods.sqlite3_extended_errcode(handle))); } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlException.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlException.cs index e5498d290f455..e469529afb3d4 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlException.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlException.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using Microsoft.CodeAnalysis.SQLite.Interop; namespace Microsoft.CodeAnalysis.SQLite.v2.Interop { diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlStatement.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlStatement.cs index c7fba97afb2ac..291a05b1ed782 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlStatement.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlStatement.cs @@ -4,8 +4,8 @@ using System; using System.Runtime.InteropServices; +using Microsoft.CodeAnalysis.SQLite.Interop; using Roslyn.Utilities; -using SQLitePCL; namespace Microsoft.CodeAnalysis.SQLite.v2.Interop { @@ -30,23 +30,23 @@ namespace Microsoft.CodeAnalysis.SQLite.v2.Interop internal struct SqlStatement { private readonly SqlConnection _connection; - private readonly sqlite3_stmt _rawStatement; + private readonly SafeSqliteStatementHandle _rawStatement; - public SqlStatement(SqlConnection connection, sqlite3_stmt statement) + public SqlStatement(SqlConnection connection, SafeSqliteStatementHandle statement) { _connection = connection; _rawStatement = statement; } internal void Close_OnlyForUseBySqlConnection() - => _connection.ThrowIfNotOk(raw.sqlite3_finalize(_rawStatement)); + => _rawStatement.Dispose(); public void Reset() - => _connection.ThrowIfNotOk(raw.sqlite3_reset(_rawStatement)); + => _connection.ThrowIfNotOk(NativeMethods.sqlite3_reset(_rawStatement)); public Result Step(bool throwOnError = true) { - var stepResult = (Result)raw.sqlite3_step(_rawStatement); + var stepResult = NativeMethods.sqlite3_step(_rawStatement); // Anything other than DONE or ROW is an error when stepping. // throw if the caller wants that, or just return the value @@ -64,30 +64,30 @@ public Result Step(bool throwOnError = true) } internal void BindStringParameter(int parameterIndex, string value) - => _connection.ThrowIfNotOk(raw.sqlite3_bind_text(_rawStatement, parameterIndex, value)); + => _connection.ThrowIfNotOk(NativeMethods.sqlite3_bind_text(_rawStatement, parameterIndex, value)); internal void BindInt64Parameter(int parameterIndex, long value) - => _connection.ThrowIfNotOk(raw.sqlite3_bind_int64(_rawStatement, parameterIndex, value)); + => _connection.ThrowIfNotOk(NativeMethods.sqlite3_bind_int64(_rawStatement, parameterIndex, value)); // SQLite PCL does not expose sqlite3_bind_blob function that takes a length. So we explicitly // DLL import it here. See https://github.com/ericsink/SQLitePCL.raw/issues/135 internal void BindBlobParameter(int parameterIndex, byte[] value, int length) - => _connection.ThrowIfNotOk(sqlite3_bind_blob(_rawStatement.ptr, parameterIndex, value, length, new IntPtr(-1))); + => _connection.ThrowIfNotOk(sqlite3_bind_blob(_rawStatement, parameterIndex, value, length, new IntPtr(-1))); [DllImport("e_sqlite3.dll", ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)] - public static extern int sqlite3_bind_blob(IntPtr stmt, int index, byte[] val, int nSize, IntPtr nTransient); + public static extern int sqlite3_bind_blob(SafeSqliteStatementHandle stmt, int index, byte[] val, int nSize, IntPtr nTransient); internal byte[] GetBlobAt(int columnIndex) - => raw.sqlite3_column_blob(_rawStatement, columnIndex); + => NativeMethods.sqlite3_column_blob(_rawStatement, columnIndex); internal int GetInt32At(int columnIndex) - => raw.sqlite3_column_int(_rawStatement, columnIndex); + => NativeMethods.sqlite3_column_int(_rawStatement, columnIndex); internal long GetInt64At(int columnIndex) - => raw.sqlite3_column_int64(_rawStatement, columnIndex); + => NativeMethods.sqlite3_column_int64(_rawStatement, columnIndex); internal string GetStringAt(int columnIndex) - => raw.sqlite3_column_text(_rawStatement, columnIndex); + => NativeMethods.sqlite3_column_text(_rawStatement, columnIndex); } } diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage.Accessor.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage.Accessor.cs index 407333d443999..a91697149efbb 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage.Accessor.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage.Accessor.cs @@ -6,6 +6,7 @@ using System.IO; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.SQLite.Interop; using Microsoft.CodeAnalysis.SQLite.v2.Interop; using Microsoft.CodeAnalysis.Storage; using Roslyn.Utilities; diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_BulkPopulateIds.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_BulkPopulateIds.cs index 68e89d37fd5e7..31288fe03e25f 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_BulkPopulateIds.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_BulkPopulateIds.cs @@ -6,6 +6,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using Microsoft.CodeAnalysis.PooledObjects; +using Microsoft.CodeAnalysis.SQLite.Interop; using Microsoft.CodeAnalysis.SQLite.v2.Interop; using Microsoft.CodeAnalysis.Storage; diff --git a/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_StringIds.cs b/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_StringIds.cs index 054fbfbfab4d9..c347baaf70342 100644 --- a/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_StringIds.cs +++ b/src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage_StringIds.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; +using Microsoft.CodeAnalysis.SQLite.Interop; using Microsoft.CodeAnalysis.SQLite.v2.Interop; using Microsoft.CodeAnalysis.Storage; using Roslyn.Utilities; From 7698b09e13605e86f18a7ad8717aca787a45b798 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 16 Apr 2020 16:53:56 -0700 Subject: [PATCH 11/11] Use DisposableDirectory for resources in AbstractPersistentStorageTests --- .../AbstractPersistentStorageTests.cs | 41 ++++++++----------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/src/VisualStudio/CSharp/Test/PersistentStorage/AbstractPersistentStorageTests.cs b/src/VisualStudio/CSharp/Test/PersistentStorage/AbstractPersistentStorageTests.cs index 5b3b64d3a00d0..08c7553803e5c 100644 --- a/src/VisualStudio/CSharp/Test/PersistentStorage/AbstractPersistentStorageTests.cs +++ b/src/VisualStudio/CSharp/Test/PersistentStorage/AbstractPersistentStorageTests.cs @@ -10,11 +10,8 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Host; -using Microsoft.CodeAnalysis.Options; -using Microsoft.CodeAnalysis.SQLite; using Microsoft.CodeAnalysis.Storage; using Microsoft.CodeAnalysis.Test.Utilities; -using Moq; using Xunit; namespace Microsoft.CodeAnalysis.UnitTests.WorkspaceServices @@ -35,7 +32,8 @@ public enum Size private readonly Encoding _encoding = Encoding.UTF8; private AbstractPersistentStorageService _storageService; - private readonly string _persistentFolder; + private readonly DisposableDirectory _persistentFolderRoot; + private readonly TempDirectory _persistentFolder; private const int LargeSize = (int)(SQLite.v2.SQLitePersistentStorage.MaxPooledByteArrayLength * 2); private const int MediumSize = (int)(SQLite.v2.SQLitePersistentStorage.MaxPooledByteArrayLength / 2); @@ -65,8 +63,8 @@ static AbstractPersistentStorageTests() protected AbstractPersistentStorageTests() { - _persistentFolder = Path.Combine(Path.GetTempPath(), PersistentFolderPrefix + Guid.NewGuid()); - Directory.CreateDirectory(_persistentFolder); + _persistentFolderRoot = new DisposableDirectory(new TempRoot()); + _persistentFolder = _persistentFolderRoot.CreateDirectory(PersistentFolderPrefix + Guid.NewGuid()); ThreadPool.GetMinThreads(out var workerThreads, out var completionPortThreads); ThreadPool.SetMinThreads(Math.Max(workerThreads, NumThreads), completionPortThreads); @@ -76,11 +74,7 @@ public void Dispose() { // This should cause the service to release the cached connection it maintains for the primary workspace _storageService?.GetTestAccessor().Shutdown(); - - if (Directory.Exists(_persistentFolder)) - { - Directory.Delete(_persistentFolder, true); - } + _persistentFolderRoot.Dispose(); } private string GetData1(Size size) @@ -158,7 +152,7 @@ public async Task PersistentService_Solution_WriteReadReopenSolution(Size size, [Theory] [CombinatorialData] - private async Task PersistentService_Solution_WriteReadSameInstance(Size size, bool withChecksum) + public async Task PersistentService_Solution_WriteReadSameInstance(Size size, bool withChecksum) { var solution = CreateOrOpenSolution(); var streamName1 = "PersistentService_Solution_WriteReadSameInstance1"; @@ -172,7 +166,7 @@ private async Task PersistentService_Solution_WriteReadSameInstance(Size size, b Assert.Equal(GetData2(size), ReadStringToEnd(await storage.ReadStreamAsync(streamName2, GetChecksum2(withChecksum)))); } - [Theory(Skip = "https://github.com/dotnet/roslyn/issues/22437")] + [Theory] [CombinatorialData] public async Task PersistentService_Project_WriteReadSameInstance(Size size, bool withChecksum) { @@ -294,8 +288,10 @@ public async Task PersistentService_Project_SimultaneousReads(Size size, bool wi [Theory] [CombinatorialData] - public async Task PersistentService_Document_SimultaneousReads(Size size, bool withChecksum) + public async Task PersistentService_Document_SimultaneousReads(Size size, bool withChecksum, [CombinatorialRange(0, 100)] int iteration) { + _ = iteration; + var solution = CreateOrOpenSolution(); var streamName1 = "PersistentService_Document_SimultaneousReads1"; @@ -477,26 +473,23 @@ private void DoSimultaneousReads(Func> read, string expectedValue) protected Solution CreateOrOpenSolution(bool nullPaths = false) { - var solutionFile = Path.Combine(_persistentFolder, "Solution1.sln"); - File.WriteAllText(solutionFile, ""); + var solutionFile = _persistentFolder.CreateOrOpenFile("Solution1.sln").WriteAllText(""); - var info = SolutionInfo.Create(SolutionId.CreateNewId(), VersionStamp.Create(), solutionFile); + var info = SolutionInfo.Create(SolutionId.CreateNewId(), VersionStamp.Create(), solutionFile.Path); var workspace = new AdhocWorkspace(); workspace.AddSolution(info); var solution = workspace.CurrentSolution; - var projectFile = Path.Combine(Path.GetDirectoryName(solutionFile), "Project1.csproj"); - File.WriteAllText(projectFile, ""); + var projectFile = _persistentFolder.CreateOrOpenFile("Project1.csproj").WriteAllText(""); solution = solution.AddProject(ProjectInfo.Create(ProjectId.CreateNewId(), VersionStamp.Create(), "Project1", "Project1", LanguageNames.CSharp, - filePath: nullPaths ? null : projectFile)); + filePath: nullPaths ? null : projectFile.Path)); var project = solution.Projects.Single(); - var documentFile = Path.Combine(Path.GetDirectoryName(projectFile), "Document1.cs"); - File.WriteAllText(documentFile, ""); + var documentFile = _persistentFolder.CreateOrOpenFile("Document1.cs").WriteAllText(""); solution = solution.AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(project.Id), "Document1", - filePath: nullPaths ? null : documentFile)); + filePath: nullPaths ? null : documentFile.Path)); // Apply this to the workspace so our Solution is the primary branch ID, which matches our usual behavior workspace.TryApplyChanges(solution); @@ -509,7 +502,7 @@ internal IChecksummedPersistentStorage GetStorage( { // If we handed out one for a previous test, we need to shut that down first _storageService?.GetTestAccessor().Shutdown(); - var locationService = new MockPersistentStorageLocationService(solution.Id, _persistentFolder); + var locationService = new MockPersistentStorageLocationService(solution.Id, _persistentFolder.Path); _storageService = GetStorageService(locationService, faultInjectorOpt); var storage = _storageService.GetStorage(solution, checkBranchId: true);