-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SQLite] Fix continuous probing for sqlite #25590
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
using System; | ||
using System.IO; | ||
using System.Runtime.InteropServices; | ||
using Microsoft.CodeAnalysis.Host; | ||
using Microsoft.CodeAnalysis.Options; | ||
using Microsoft.CodeAnalysis.Shared.Utilities; | ||
|
@@ -19,6 +20,47 @@ internal partial class SQLitePersistentStorageService : AbstractPersistentStorag | |
|
||
private readonly IPersistentStorageFaultInjector _faultInjectorOpt; | ||
|
||
[DllImport("kernel32.dll")] | ||
private static extern IntPtr LoadLibrary(string dllToLoad); | ||
|
||
internal static bool TryInitializeLibraries() => s_initialized.Value; | ||
|
||
private static readonly Lazy<bool> s_initialized = new Lazy<bool>(() => TryInitializeLibrariesLazy()); | ||
|
||
private static bool TryInitializeLibrariesLazy() | ||
{ | ||
// Attempt to load the correct version of e_sqlite.dll. That way when we call | ||
// into SQLitePCL.Batteries_V2.Init it will be able to find it. | ||
// | ||
// Only do this on Windows when we can safely do the LoadLibrary call to this | ||
// direct dll. On other platforms, it is the responsibility of the host to ensure | ||
// that the necessary sqlite library has already been loaded such that SQLitePCL.Batteries_V2 | ||
// will be able to call into it. | ||
if (Environment.OSVersion.Platform == PlatformID.Win32NT) | ||
{ | ||
var myFolder = Path.GetDirectoryName( | ||
typeof(SQLitePersistentStorage).Assembly.Location); | ||
|
||
var is64 = IntPtr.Size == 8; | ||
var subfolder = is64 ? "x64" : "x86"; | ||
|
||
LoadLibrary(Path.Combine(myFolder, subfolder, "e_sqlite3.dll")); | ||
} | ||
|
||
try | ||
{ | ||
// Necessary to initialize SQLitePCL. | ||
SQLitePCL.Batteries_V2.Init(); | ||
} | ||
catch (Exception e) when (e is DllNotFoundException || e is EntryPointNotFoundException) | ||
{ | ||
StorageDatabaseLogger.LogException(e); | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
public SQLitePersistentStorageService( | ||
IOptionService optionService, | ||
IPersistentStorageLocationService locationService, | ||
|
@@ -66,7 +108,17 @@ protected override bool TryOpenDatabase( | |
} | ||
catch (Exception) | ||
{ | ||
sqlStorage?.Dispose(); | ||
if (sqlStorage != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if exception was thrown, shouldn't we always dispose the owner lock? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the lock is to make sure we own sqlite file, so if we dont have it (since it is either null or disposed here), then I think we should let it go? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah. just realized that the dispose of the storage will dispose the lock. sorry about that. |
||
{ | ||
// Dispose of the storage, releasing the ownership lock. | ||
sqlStorage.Dispose(); | ||
} | ||
else | ||
{ | ||
// The storage was not created so nothing owns the lock. | ||
// Dispose the lock to allow reuse. | ||
dbOwnershipLock.Dispose(); | ||
} | ||
throw; | ||
} | ||
|
||
|
@@ -98,10 +150,7 @@ private static void EnsureDirectory(string databaseFilePath) | |
Directory.CreateDirectory(directory); | ||
} | ||
|
||
protected override bool ShouldDeleteDatabase(Exception exception) | ||
{ | ||
// Error occurred when trying to open this DB. Try to remove it so we can create a good dB. | ||
return true; | ||
} | ||
// Error occurred when trying to open this DB. Try to remove it so we can create a good DB. | ||
protected override bool ShouldDeleteDatabase(Exception exception) => true; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like, at the top of this method, we would see in the first place if we had been able to load sqlite. And, if not, we wouldn't do any work. no need for locks, exception handling, and wahtnot in that case. Basically, we shouldn't even be trying this complex logic if we can't even get at sqlite in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't even called if we haven't been able to load sqlite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Therzok the tests probably are failing because some unrelated tests ran before the Sqlite tests and set the static field to false and Sqlite tests are bailing out due to the static field already being set as false.
I bet before, except Sqlite tests, other conditions made Sqlite service to bail out before creating SqliteStorage (so no static ctor get called) and let the tests to actually run.
...
I think either you take out Sqlite persistent service out from default test MEF so that only SQLite test uses real Sqlite persistent service, or addin test checking code somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or you don't save the flag as you did :) that will work as well.