Code CLI - Add --state filter to --list-extensions #93967

lszomoru · 2020-03-31T15:18:38Z

This PR fixes #19083

With this change I have added the --state filter to the --list-extensions command. The filter only uses global enablement state (extension is globally enabled/disabled). In order to use the global enablement service I had to connect to the SQLLite database to get the list of disabled extensions. Not sure if that is the right approach here so any guidance would be highly appreciated. Thanks!

Usage:
code --list-extensions --state enabled - will list all extensions that are enabled
code --list-extensions --state disabled - will list all extensions that are disabled

lszomoru · 2020-03-31T15:20:46Z

tagging @joaomoreno, @bpasero, @sandy081 to sign off on the change

bpasero

I talked with @joaomoreno about this earlier today. We are using SQLite as a DB for storing state. Normally things should be OK accessing the DB from CLI if no other process is running. However, in case VSCode is already running, we now might have 2 processes connecting to the same DB on disk. We do not run SQLite in a mode that locks the DB for single-process use, but I fear this may have a chance to cause corruption if not carefully understood how SQLite behaves in this case.

Couple of thoughts:

can we provide an implementation of SQLite state access that is guaranteed to not brick an instance that is accessed meanwhile from another process (e.g. with some options)
could the CLI process first check if it is the first one or not and then delegate the query to the first instance?

I would probably go with the first option and implement a readonly version of SQLiteStorageDatabase. Still, we need to understand how SQLite behaves.

Sorry, #19083 should have not been marked as help wanted so easily.

sandy081 · 2020-03-31T19:03:27Z

I would go for second option

could the CLI process first check if it is the first one or not and then delegate the query to the first instance?

Because there are other use case to update storage from CLI - #63903

Check the same suggestion from @joaomoreno here - #63903 (comment)

lszomoru · 2020-03-31T19:25:06Z

no worries about the issue being marked as "help wanted", and thanks for the feedback.

Implementing a "read-only" option for SQLiteStorageDatabase seems a reasonable option which would unblock the completion of this issue however as @sandy081 calls it out there are several other scenarios for which the CLI needs write access. If we were to add storage support for the CLI we should do it in a way that fits both read and write scenario so @joaomoreno's suggestion makes sense to me especially that there is already an established pattern. I can take a stab at it and see how far I can get.

Is there any particular reason why we open a connection to the SQLLite database and keep it open for the lifetime of the process? Is there such a big overhead for establishing the connection? I am asking as another option which would save us from the interprocess communication would be for each process to open the connection/execute query/close the connection. I am also reviewing the SQLLite documentation for concurrency/locking.

joaomoreno · 2020-04-01T05:42:38Z

Is there any particular reason why we open a connection to the SQLLite database and keep it open for the lifetime of the process? Is there such a big overhead for establishing the connection? I am asking as another option which would save us from the interprocess communication would be for each process to open the connection/execute query/close the connection. I am also reviewing the SQLLite documentation for concurrency/locking.

This would definitely be way too far, just to tackle this issue.

I would even argue read/write access is taking it too far. Yes, it would be great to have, but no current feature request is too pressing to force this amount of change.

I would investigate on having concurrent read-only access to the db, which after a few web searches, definitely seems like a feature SQLite supports. This would not only fix this feature, unblock others and would keep things simple.

sandy081 · 2020-04-01T06:12:42Z

Agreed that implementing read & write is too much for this issue. But there are other issues which need write access too.

Ability to enable / disable from CLI - Allow disabling builtin extensions from the command line and persisting the configuration state so that subsequently when vscode is closed and open ithose extensions will still be disabled #52639 (comment)
Support pinning extension version when installed from CLI #63903

If we started thinking and investing on this, is not it recommended to think about the complete solution? I do not understand if the complete solution is technically feasible or not.

Also this will make storage service inconsistent (there by other services like extension enablements too) across the processes we have.

lszomoru · 2020-04-01T08:02:10Z

As the issue in question is 3 years old, I do not believe that there is an incentive for a tactical change in order to implement the requested feature. In light of that, I would like to advocate for the right solution which will unblock all request that are currently blocked due to the fact that the CLI does not have storage access.

Given that there is already a pattern for delegating storage queries, I will go ahead and explore that. At a later date we can explore keeping the connection open just for the time of executing the query since that seems to be the best solution long term.

lszomoru · 2020-04-03T21:06:32Z

I had a quick chat with @joaomoreno earlier today and we have agreed that for the scope of this particular issue we will go with adding the capability to create a read-only connection to the SQLite database. This works as expected however there is one edge case where this will not work. If the global SQLite database does not exist and the CLI is run that it will fail as we cannot run the script to create the database when we are using a read-only connection. If you feel that we should address this edge case, we should discuss further. Thanks!

bpasero · 2020-04-06T08:40:59Z

@lszomoru are you able to resolve the conflicts? sorry, I pushed a lot of refactorings around environment

lszomoru · 2020-04-06T08:47:29Z

@bpasero, sure. Let me resolve the conflicts and update the pull request.

lszomoru · 2020-04-06T09:11:10Z

@bpasero I have resolved the conflicts and updated the pull request.

bpasero

A bit of feedback, I focused on the DB pieces 👍

bpasero · 2020-04-06T11:45:45Z

src/vs/base/parts/storage/node/storage.ts

@@ -20,6 +20,7 @@ interface IDatabaseConnection {
 }

 export interface ISQLiteStorageDatabaseOptions {
+	readonly intent?: number;


I would prefer if we simply had the option to open the DB as readonly and not expose the underlying SQLite modes as option. I think in the end we just have 1 client that wants to open the DB readonly so I would optimise for that scenario.

bpasero · 2020-04-06T11:46:15Z

src/vs/base/parts/storage/node/storage.ts

@@ -34,6 +35,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {

 	get onDidChangeItemsExternal(): Event<IStorageItemsChangeEvent> { return Event.None; } // since we are the only client, there can be no external changes

+	private static readonly DEFAULT_MODE = OPEN_READWRITE | OPEN_CREATE; // default mode when connecting to the DB


I would prefer if we do not use any default mode but leave that up to the SQLite implementation. We only set the mode to readonly when instructed, but otherwise leave it as is.

bpasero · 2020-04-06T11:46:40Z

src/vs/base/parts/storage/node/storage.ts

@@ -297,11 +299,16 @@ export class SQLiteStorageDatabase implements IStorageDatabase {
 		return new Promise((resolve, reject) => {
 			import('vscode-sqlite3').then(sqlite3 => {
 				const connection: IDatabaseConnection = {
-					db: new (this.logger.isTracing ? sqlite3.verbose().Database : sqlite3.Database)(path, error => {
+					db: new (this.logger.isTracing ? sqlite3.verbose().Database : sqlite3.Database)(path, this.options.intent ?? SQLiteStorageDatabase.DEFAULT_MODE, error => {


As mentioned above, pass undefined instead of default mode to leave the default up to the underlying library.

bpasero · 2020-04-06T11:46:51Z

src/vs/base/parts/storage/node/storage.ts

 						if (error) {
 							return connection.db ? connection.db.close(() => reject(error)) : reject(error);
 						}

+						// Return the connection when using OPEN_READONLY
+						if (this.options && this.options.intent && (this.options.intent & OPEN_READONLY) === OPEN_READONLY) {
+							resolve(connection);


You are missing a return here and as such resolve the promise twice.

bpasero · 2020-04-06T11:48:06Z

src/vs/code/node/cliProcessMain.ts

+			}
+
+			// Get the list of disabled extensions
+			const disabledExtensions = await this.globalExtensionEnablementService.getDisabledExtensions();


await not needed here

bpasero · 2020-04-06T11:49:06Z

src/vs/code/node/cliProcessMain.ts

@@ -293,6 +325,7 @@ export class Main {
 }

 const eventPrefix = 'monacoworkbench';
+const globalStorageName = 'state.vscdb';


Extract to a place where it can be reused?

bpasero · 2020-04-06T11:50:23Z

src/vs/code/node/cliProcessMain.ts

+	const storageService = new NativeStorageService(storageDatabase, logService, environmentService);
+	await storageService.initialize();
+	services.set(IStorageService, storageService);
+	disposables.add(toDisposable(() => storageService.flush()));


This is not needed as flush is only used for writing

bpasero · 2020-04-06T11:50:34Z

src/vs/code/node/cliProcessMain.ts

@@ -306,6 +339,13 @@ export async function main(argv: ParsedArgs): Promise<void> {
 	await Promise.all<void | undefined>([environmentService.appSettingsHome.fsPath, environmentService.extensionsPath]
 		.map((path): undefined | Promise<void> => path ? mkdirp(path) : undefined));

+	// Storage


This deserves a bit more documentation why the DB can only be opened readonly.

sandy081 · 2020-04-06T15:25:56Z

Discussed with @joaomoreno and @lszomoru and I do not think this is worth adding this feature for the amount of work that is done and the debt it might create.

Also the solution might not be complete.. we also have workspace enabled extensions. So showing the global enablement might not be complete enough
and adding this is asking for more trouble.

So, I would recommend to abandon this. Sorry @lszomoru for tagging this as help-wanted request .

Add --state filter to --list-extensions

5aada08

jrieken assigned joaomoreno Mar 31, 2020

bpasero reviewed Mar 31, 2020

View reviewed changes

sandy081 added the extensions Issues concerning extensions label Apr 1, 2020

Use a read-only connection from the CLI

b5d6e2f

lszomoru added 2 commits April 3, 2020 23:15

Refactored code to be less invasive

6fc2b3e

Fixed a regression

403a811

sandy081 self-assigned this Apr 6, 2020

lszomoru added 2 commits April 6, 2020 11:03

Merge remote-tracking branch 'upstream/master'

186a5d4

Add missing file

742d2c3

bpasero requested changes Apr 6, 2020

View reviewed changes

lszomoru closed this Apr 15, 2020

github-actions bot locked and limited conversation to collaborators May 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code CLI - Add --state filter to --list-extensions #93967

Code CLI - Add --state filter to --list-extensions #93967

lszomoru commented Mar 31, 2020

lszomoru commented Mar 31, 2020

bpasero left a comment

sandy081 commented Mar 31, 2020

lszomoru commented Mar 31, 2020

joaomoreno commented Apr 1, 2020 •

edited

Loading

sandy081 commented Apr 1, 2020

lszomoru commented Apr 1, 2020

lszomoru commented Apr 3, 2020

bpasero commented Apr 6, 2020

lszomoru commented Apr 6, 2020

lszomoru commented Apr 6, 2020

bpasero left a comment

bpasero Apr 6, 2020

bpasero Apr 6, 2020

bpasero Apr 6, 2020

bpasero Apr 6, 2020

bpasero Apr 6, 2020

bpasero Apr 6, 2020

bpasero Apr 6, 2020

bpasero Apr 6, 2020

sandy081 commented Apr 6, 2020 •

edited

Loading

		@@ -34,6 +35,7 @@ export class SQLiteStorageDatabase implements IStorageDatabase {

		get onDidChangeItemsExternal(): Event<IStorageItemsChangeEvent> { return Event.None; } // since we are the only client, there can be no external changes

		private static readonly DEFAULT_MODE = OPEN_READWRITE \| OPEN_CREATE; // default mode when connecting to the DB

Code CLI - Add --state filter to --list-extensions #93967

Code CLI - Add --state filter to --list-extensions #93967

Conversation

lszomoru commented Mar 31, 2020

lszomoru commented Mar 31, 2020

bpasero left a comment

Choose a reason for hiding this comment

sandy081 commented Mar 31, 2020

lszomoru commented Mar 31, 2020

joaomoreno commented Apr 1, 2020 • edited Loading

sandy081 commented Apr 1, 2020

lszomoru commented Apr 1, 2020

lszomoru commented Apr 3, 2020

bpasero commented Apr 6, 2020

lszomoru commented Apr 6, 2020

lszomoru commented Apr 6, 2020

bpasero left a 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

sandy081 commented Apr 6, 2020 • edited Loading

joaomoreno commented Apr 1, 2020 •

edited

Loading

sandy081 commented Apr 6, 2020 •

edited

Loading