[SQLite] Fix continuous probing for sqlite #25590

Therzok · 2018-03-19T11:52:13Z

The static constructor of SQLitePersistentStorage would throw an
exception, which would then get logged by the exception logger.

This changes the following logic:

Don't delete the database if e_sqlite is not found.
Disable persistent storage if e_sqlite is not found.
Releases the db ownership lock if e_sqlite is not found.

Fixes #24042

Extra story:
There is a architectures of e_sqlite not existing for some platforms,
i.e. ARM64. Therefore, in MonoDevelop, it would throw a
TypeInitializationException on trying to initialize the SQLitePersistentStorage.

In this case, what would happen is:

The persistent storage service would create the lock file.
It would throw in the static constructor, without having passed
ownership
Thus, the lock file is created by the service, not held by anyone
specifically
The exception wasn't accounted for, so the storage file would be
deleted
We now have a lock file present and no storage created
Roslyn will keep trying to create the persistent storage on every
subsequent try, ending up in a performance issue caused by continuous
IO on trying to get ownership on the lock file, but it can't take it
because nobody released the old lock

Therefore, shortcircuiting and disabling persistent storage in case the
lib is not found is better.

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

Therzok · 2018-03-19T11:52:57Z

cc @sharwell @CyrusNajmabadi 👀

@jasonmalinowski is there a way to get nupkgs for this PR so I can test this locally?

Therzok · 2018-03-19T12:33:39Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

@@ -64,6 +64,13 @@ protected override string GetDatabaseFilePath(string workingFolderPath)
                sqlStorage.Initialize(solution);

            }
+            catch (Exception ex) when (ex is DllNotFoundException || ex is EntryPointNotFoundException)


Need to catch TypeInitializationException and check InnerException for the data and throw that

sharwell · 2018-03-21T14:15:26Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

+                // sqlStorage was not instantiated yet, due to an exception in the static constructor.
+                dbOwnershipLock.Dispose();
+                DisableStorage();
+                throw ex.InnerException;


❓ Why not just throw; here?

The exception thrown from here propagates into ShouldDeleteDatabase, via here:
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces.Desktop/Workspace/Storage/AbstractPersistentStorageService.cs,184

Maybe throw; is the better option here, and the check in ShouldDeleteDatabase should be done for TypeInitializationException and its inner exception.

We're masking an exception and changing its stacktrace, so yes, I'll fix this.

💭 It seems like the less risky change in the absence of a need to drop the outer exception. I could go either way, let me know which one you will go with.

sharwell · 2018-03-21T15:01:18Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

@@ -98,10 +114,23 @@ private static void EnsureDirectory(string databaseFilePath)
            Directory.CreateDirectory(directory);
        }

+        static bool IsNativeLibraryResolutionException(Exception ex)


💡 Missing access modifier (will send PR to add code style rule so this is more apparent)

sharwell · 2018-03-21T15:03:49Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

+                ex = typeInitializationException.InnerException;
+            }
+
+            return exception is DllNotFoundException || exception is EntryPointNotFoundException;


❗️ ~~exception~~ ex

heejaechang · 2018-03-21T18:54:20Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

            {
-                sqlStorage?.Dispose();
+                if (IsNativeLibraryResolutionException(exception))


rather than doing this way, can we add explicit check for those dependencies? basically, rather than doing this implicitly through static constructor (http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces.Desktop/Workspace/SQLite/SQLitePersistentStorage.cs,96)

make an explicit static API for it such as bool SQLitePersistentStorage.InitLibraries(). which will run the LoadLibrary once and cache the result. and next time it is called, it just return true or false.

and in TryOpenDatabase, before TryGetDatabaseOwnership, it calls SQLitePersistentStorage.InitiLibrary and if that return false, return storage = null and return false like what dbOwnershipLock does.

I think that is more explicit check and follow pattern already in this method.

...

if you are worry about same thing repeatedly get called, then, I think that check should move up to here.
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces.Desktop/Workspace/Storage/PersistenceStorageServiceFactory.cs,24

since the condition where that LoadLibrary will fail is process wide, not per workspace or solution. the factory itself should check such condition and return
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Workspace/Host/PersistentStorage/NoOpPersistentStorageService.cs,0ec32e8cdd068f49

since in the process, SQLite will be never supported.

Yeah, that's a better solution. This PR ended up being bigger than I initially imagined 😛

I'll open a PR writing it way.

Therzok · 2018-03-23T15:07:28Z

This came up so testing doesn't work

error MSB6002: The command-line for the "CreatePkgDef" task is too long. Command-lines longer than 32000 characters are likely to fail. Try reducing the length of the command-line by breaking down the call to "CreatePkgDef" into multiple calls with fewer parameters per call. [D:\j\workspace\windows_relea---233a1cda\src\VisualStudio\IntegrationTest\TestSetup\VisualStudioIntegrationTestSetup.csproj]

Therzok · 2018-03-25T17:36:06Z

Hmmm, so I think I'm hitting a rather peculiar bug right now. It seems to say that it can't probe for e_sqlite.dll in either x86/x64 dir relative to the workspaces.dll, but that should definitely be copied over.

Any help here? I don't really understand how this could've passed before, except for a hidden exception in FatalError/DatabaseLogger

Therzok · 2018-03-25T18:29:15Z

Ah, I think I figured it out.

heejaechang · 2018-03-27T22:43:04Z

@dotnet/roslyn-infrastructure is our jenkins currently having some issues?

heejaechang · 2018-03-27T22:47:00Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

@@ -66,7 +104,17 @@ protected override string GetDatabaseFilePath(string workingFolderPath)
            }
            catch (Exception)
            {
-                sqlStorage?.Dispose();
+                if (sqlStorage != null)


if exception was thrown, shouldn't we always dispose the owner lock?

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?

ah. just realized that the dispose of the storage will dispose the lock. sorry about that.

heejaechang · 2018-03-27T22:49:05Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

@@ -100,7 +148,7 @@ private static void EnsureDirectory(string databaseFilePath)

        protected override bool ShouldDeleteDatabase(Exception exception)


nit: convert to => ?

heejaechang

you are good. just realized that dispose of sqlite storage will dispose the lock.

jinujoseph · 2018-03-27T23:23:22Z

@Pilchie for ask mode approval for 15.7 preview 4

Pilchie · 2018-03-28T02:16:18Z

Approved - pending passing unit test runs...

Therzok · 2018-03-28T06:49:53Z

Rebasing again hoping they pass...

sharwell · 2018-03-28T11:57:20Z

@Therzok Looks like the 64-bit tests have a legitimate failure

CyrusNajmabadi · 2018-03-28T18:06:35Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

+
+            lock (_gate)
+            {
+                if (_initialized)


please don't do double checked locking. i still don't trust that .net has a consistent story around reordered writes and whatnot to make this safe. It's trivial, and no perf problem, to just take the lock and do the check.

Oh, this part was just for testing. This is not needed after all.

CyrusNajmabadi · 2018-03-28T18:07:32Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

+        [DllImport("kernel32.dll")]
+        private static extern IntPtr LoadLibrary(string dllToLoad);
+        private static bool _initialized;
+        private static object _gate = new object();


it might make a lot of sense to extract all this code related to laoding sqlite and put it in its own class. The class would expose almost nothing except a bit saying if you can use sqlite or not. You would never have to care or look for exceptions relating to asking the question in teh first place if you can use it.

CyrusNajmabadi · 2018-03-28T18:08:04Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

+                if (_initialized)
+                {
+                    return true;
+                }


i feel like the momeent after checking, we should set _initialized to true, so we never execute this block more than once.

Instead of just returning 'true', i feel a better API would be to have it actually have a different bool stating if we actually initialized properly or not. (Alternatively, use a bool? where 'null' means 'have not computed', and non-null is the actual value.)

CyrusNajmabadi · 2018-03-28T18:09:29Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

+                catch (Exception e) when (e is DllNotFoundException || e is EntryPointNotFoundException)
+                {
+                    StorageDatabaseLogger.LogException(e);
+                    return false;


i really dislike that we haven't marked _iniitialized here. so this can just be hit over and over again.

This was just to test whether unit tests passed with this. I used to have a Lazy<bool> in an earlier version, but that broke unit tests in the same way they are broken now.

CyrusNajmabadi · 2018-03-28T18:10:31Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

@@ -66,7 +120,17 @@ protected override string GetDatabaseFilePath(string workingFolderPath)
            }
            catch (Exception)
            {
-                sqlStorage?.Dispose();


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.

This isn't even called if we haven't been able to load sqlite.

@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.

or you don't save the flag as you did :) that will work as well.

heejaechang · 2018-03-28T19:15:12Z

@Therzok alright, test is still failing. so I looked through tests. looks like Sqlite tests bypass Factory and create Sqlite service directly. so there is no one who initialize sqlite. I think that is why your test is failing.

put the initialize in here
http://source.roslyn.io/#Roslyn.VisualStudio.CSharp.UnitTests/PersistentStorage/SQLitePersistentStorageTests.cs,18

you probably can add it as static ctor of the test. and I think you can put back the flag to make it not run twice if you want to. looks like test MEF doesn't include Sqlite service so static flag shouldn't be a problem.

Therzok · 2018-03-29T07:02:07Z

Omg, thanks! I missed that! Will fix!

heejaechang · 2018-03-29T08:17:38Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorageService.cs

+
+        internal static bool TryInitializeLibraries() => _initialized.Value;
+
+        private static Lazy<bool> _initialized = new Lazy<bool>(() => TryInitializeLibrariesLazy());


readonly and s_initialized

The static constructor of SQLitePersistentStorage would throw an exception, which would then get logged by the exception logger. This changes the following logic: * The database used to be deleted if e_sqlite was not found. * The sqlite persistent storage is not used if e_sqlite is not found. * The database ownership lock is released if any exceptions happen while trying to instantiate the storage. Fixes dotnet#24042 Extra story: There is a architectures of e_sqlite not existing for some platforms, i.e. ARM64. Therefore, in MonoDevelop, it would throw a TypeInitializationException on trying to initialize the SQLitePersistentStorage. In this case, what would happen is: * The persistent storage service would create the lock file. * It would throw in the static constructor, without having passed ownership * Thus, the lock file is created by the service, not held by anyone specifically * The exception wasn't accounted for, so the storage file would be deleted * We now have a lock file present and no storage created * Roslyn will keep trying to create the persistent storage on every subsequent try, ending up in a performance issue caused by continuous IO on trying to get ownership on the lock file, but it can't take it because nobody released the old lock Therefore, shortcircuiting and disabling persistent storage in case the lib is not found is better.

Therzok · 2018-03-30T17:15:16Z

Thanks!

Therzok requested a review from a team as a code owner March 19, 2018 11:52

Therzok commented Mar 19, 2018

View reviewed changes

Therzok force-pushed the fix24042 branch from f451581 to 9ff7e73 Compare March 19, 2018 13:12

Therzok mentioned this pull request Mar 19, 2018

monodevelop fails to load projects due to sqlite3 issue on Ubuntu 17.10 mono/monodevelop#4224

Open

Therzok force-pushed the fix24042 branch 3 times, most recently from 10c2346 to 1b7537a Compare March 19, 2018 17:54

sharwell requested changes Mar 21, 2018

View reviewed changes

Therzok force-pushed the fix24042 branch from 1b7537a to 8a348f0 Compare March 21, 2018 14:56

sharwell requested changes Mar 21, 2018

View reviewed changes

Therzok force-pushed the fix24042 branch 2 times, most recently from 47064db to 9badc11 Compare March 21, 2018 15:10

sharwell approved these changes Mar 21, 2018

View reviewed changes

sharwell added this to the 15.7 milestone Mar 21, 2018

heejaechang reviewed Mar 21, 2018

View reviewed changes

Therzok force-pushed the fix24042 branch 5 times, most recently from 31a50b5 to 8fb8ada Compare March 23, 2018 13:18

Therzok force-pushed the fix24042 branch 3 times, most recently from 3deceba to 17141e1 Compare March 25, 2018 17:17

Therzok force-pushed the fix24042 branch from 5912ce8 to 24551c6 Compare March 25, 2018 19:43

heejaechang reviewed Mar 27, 2018

View reviewed changes

heejaechang approved these changes Mar 27, 2018

View reviewed changes

Pilchie added the Approved to merge label Mar 28, 2018

Therzok force-pushed the fix24042 branch from 24551c6 to 007a593 Compare March 28, 2018 06:50

CyrusNajmabadi reviewed Mar 28, 2018

View reviewed changes

Therzok force-pushed the fix24042 branch from ae1d6c8 to 007a593 Compare March 28, 2018 18:37

Therzok force-pushed the fix24042 branch from 007a593 to caf1a4a Compare March 29, 2018 07:10

heejaechang reviewed Mar 29, 2018

View reviewed changes

Therzok force-pushed the fix24042 branch from caf1a4a to b7b1d49 Compare March 29, 2018 09:33

sharwell merged commit b6b0f03 into dotnet:dev15.7.x Mar 30, 2018

Therzok deleted the fix24042 branch March 30, 2018 17:15

sharwell mentioned this pull request Apr 19, 2018

Defer loading of SQLite binaries until they will be used #26269

Merged

		@@ -100,7 +148,7 @@ private static void EnsureDirectory(string databaseFilePath)

		protected override bool ShouldDeleteDatabase(Exception exception)


		internal static bool TryInitializeLibraries() => _initialized.Value;

		private static Lazy<bool> _initialized = new Lazy<bool>(() => TryInitializeLibrariesLazy());

[SQLite] Fix continuous probing for sqlite #25590

[SQLite] Fix continuous probing for sqlite #25590

Conversation

Therzok commented Mar 19, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Therzok commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Therzok commented Mar 23, 2018

Therzok commented Mar 25, 2018

Therzok commented Mar 25, 2018

heejaechang commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang left a comment • edited Loading

Choose a reason for hiding this comment

jinujoseph commented Mar 27, 2018

Pilchie commented Mar 28, 2018

Therzok commented Mar 28, 2018

sharwell commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Mar 28, 2018 • edited Loading

Therzok commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Therzok commented Mar 30, 2018

Therzok commented Mar 19, 2018 •

edited

Loading

sharwell Mar 21, 2018 •

edited

Loading

heejaechang Mar 21, 2018 •

edited

Loading

heejaechang left a comment •

edited

Loading

heejaechang commented Mar 28, 2018 •

edited

Loading