From b9a4b2b58f8e9adbd903e8504637c29ec8922c86 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Fri, 27 May 2022 11:40:59 -0400 Subject: [PATCH] (core) add missing tsconfig file that affects IDEs Summary: I missed committing a file that is important for editing files comfortably in the ext directory in an IDE. This diff: * Adds tsconfig-base-ext.json - that was the only intended change * Unrelated: Forces all creation of connections to the home db through a new `getOrCreateConnection` method which changes the `busy_timeout` if using Sqlite. This was an attempt to fix random "database is locked" test failures. I believe multiple connections to the home db as an sqlite file do not happen in self-hosted Grist (where there is a single node process) or in our SaaS (where the database is in postgres). It does affect Grist started using `devServerMain.ts` (where multiple processes accessing same database are started) or various test configurations when extra database connections are opened. * Unrelated: I added a `busy_timeout` for session storage, when it uses Sqlite. Again, I don't believe this affects self-hosted Grist or our SaaS. * Tweaked a `BillingDiscount` test that looked perhaps vulnerable to a stripe request stalling. I can't be sure my tweaks actually help, since I didn't succeed in replicating the failures. Update: looks like the "locked" error can still happen :( Test Plan: manual Reviewers: jarek Reviewed By: jarek Subscribers: jarek Differential Revision: https://phab.getgrist.com/D3450 --- app/gen-server/lib/HomeDBManager.ts | 12 ++--------- app/server/lib/dbUtils.ts | 33 +++++++++++++++++++++++++++-- app/server/lib/gristSessions.ts | 10 ++++++++- test/gen-server/seed.ts | 16 +++++++------- test/testUtils.ts | 8 ------- 5 files changed, 50 insertions(+), 29 deletions(-) diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts index d89d44d4..dfe43f2b 100644 --- a/app/gen-server/lib/HomeDBManager.ts +++ b/app/gen-server/lib/HomeDBManager.ts @@ -50,6 +50,7 @@ import { now, readJson } from 'app/gen-server/sqlUtils'; +import {getOrCreateConnection} from 'app/server/lib/dbUtils'; import {makeId} from 'app/server/lib/idUtils'; import * as log from 'app/server/lib/log'; import {Permit} from 'app/server/lib/Permit'; @@ -60,10 +61,8 @@ import {Request} from "express"; import { Brackets, Connection, - createConnection, DatabaseType, EntityManager, - getConnection, SelectQueryBuilder, WhereExpression } from "typeorm"; @@ -345,14 +344,7 @@ export class HomeDBManager extends EventEmitter { } public async connect(): Promise { - try { - // If multiple servers are started within the same process, we - // share the database connection. This saves locking trouble - // with Sqlite. - this._connection = getConnection(); - } catch (e) { - this._connection = await createConnection(); - } + this._connection = await getOrCreateConnection(); this._dbType = this._connection.driver.options.type; } diff --git a/app/server/lib/dbUtils.ts b/app/server/lib/dbUtils.ts index c0cf76fc..6a81dcec 100644 --- a/app/server/lib/dbUtils.ts +++ b/app/server/lib/dbUtils.ts @@ -1,5 +1,6 @@ import {synchronizeProducts} from 'app/gen-server/entity/Product'; -import {Connection, createConnection} from 'typeorm'; +import {Mutex} from 'async-mutex'; +import {Connection, createConnection, getConnection} from 'typeorm'; // Summary of migrations found in database and in code. interface MigrationSummary { @@ -38,11 +39,39 @@ export async function getMigrations(connection: Connection): Promise { + return connectionMutex.runExclusive(async() => { + try { + // If multiple servers are started within the same process, we + // share the database connection. This saves locking trouble + // with Sqlite. + return getConnection(); + } catch (e) { + const connection = await createConnection(); + // When using Sqlite, set a busy timeout of 3s to tolerate a little + // interference from connections made by tests. Logging doesn't show + // any particularly slow queries, but bad luck is possible. + // This doesn't affect when Postgres is in use. It also doesn't have + // any impact when there is a single connection to the db, as is the + // case when Grist is run as a single process. + if (connection.driver.options.type === 'sqlite') { + await connection.query('PRAGMA busy_timeout = 3000'); + } + return connection; + } + }); +} + export async function runMigrations(connection: Connection) { // on SQLite, migrations fail if we don't temporarily disable foreign key // constraint checking. This is because for sqlite typeorm copies each diff --git a/app/server/lib/gristSessions.ts b/app/server/lib/gristSessions.ts index 70301b92..bd64f547 100644 --- a/app/server/lib/gristSessions.ts +++ b/app/server/lib/gristSessions.ts @@ -73,8 +73,16 @@ function createSessionStoreFactory(sessionsDB: string): () => SessionStore { const store = new SQLiteStore({ dir: path.dirname(sessionsDB), db: path.basename(sessionsDB), // SQLiteStore no longer appends a .db suffix. - table: 'sessions' + table: 'sessions', }); + // In testing, and monorepo's "yarn start", session is accessed from multiple + // processes, so could hit lock failures. + // connect-sqlite3 has a concurrentDb: true flag that can be set, but + // it puts the database in WAL mode, which would have implications + // for self-hosters (a second file to think about). Instead we just + // set a busy timeout. + store.db.run('PRAGMA busy_timeout = 1000'); + return assignIn(store, { async close() {}}); }; } diff --git a/test/gen-server/seed.ts b/test/gen-server/seed.ts index 7e7594a0..3c12e094 100644 --- a/test/gen-server/seed.ts +++ b/test/gen-server/seed.ts @@ -25,7 +25,7 @@ import {addPath} from 'app-module-path'; import {IHookCallbackContext} from 'mocha'; import * as path from 'path'; -import {Connection, createConnection, getConnectionManager, Repository} from 'typeorm'; +import {Connection, getConnectionManager, Repository} from 'typeorm'; if (require.main === module) { addPath(path.dirname(path.dirname(__dirname))); @@ -42,7 +42,7 @@ import {User} from "app/gen-server/entity/User"; import {Workspace} from "app/gen-server/entity/Workspace"; import {EXAMPLE_WORKSPACE_NAME} from 'app/gen-server/lib/HomeDBManager'; import {Permissions} from 'app/gen-server/lib/Permissions'; -import {runMigrations, undoLastMigration, updateDb} from 'app/server/lib/dbUtils'; +import {getOrCreateConnection, runMigrations, undoLastMigration, updateDb} from 'app/server/lib/dbUtils'; import {FlexServer} from 'app/server/lib/FlexServer'; import * as fse from 'fs-extra'; @@ -512,7 +512,7 @@ export async function createInitialDb(connection?: Connection, migrateAndSeedDat // sqlite db is in memory (":memory:") there's nothing to delete. const uncommitted = !connection; // has user already created a connection? // if so we won't be able to delete sqlite db - connection = connection || await createConnection(); + connection = connection || await getOrCreateConnection(); const opt = connection.driver.options; if (process.env.TEST_CLEAN_DATABASE) { if (opt.type === 'sqlite') { @@ -527,7 +527,7 @@ export async function createInitialDb(connection?: Connection, migrateAndSeedDat if (await fse.pathExists(database)) { await fse.unlink(database); } - connection = await createConnection(); + connection = await getOrCreateConnection(); } } else if (opt.type === 'postgres') { // recreate schema, destroying everything that was inside it @@ -555,7 +555,7 @@ export async function addSeedData(connection: Connection) { } export async function createBenchmarkDb(connection?: Connection) { - connection = connection || await createConnection(); + connection = connection || await getOrCreateConnection(); await updateDb(connection); await connection.transaction(async tr => { const seed = new Seed(tr.connection); @@ -629,18 +629,18 @@ async function main() { await createInitialDb(); return; } else if (cmd === 'benchmark') { - const connection = await createConnection(); + const connection = await getOrCreateConnection(); await createInitialDb(connection, false); await createBenchmarkDb(connection); return; } else if (cmd === 'migrate') { process.env.TYPEORM_LOGGING = 'true'; - const connection = await createConnection(); + const connection = await getOrCreateConnection(); await runMigrations(connection); return; } else if (cmd === 'revert') { process.env.TYPEORM_LOGGING = 'true'; - const connection = await createConnection(); + const connection = await getOrCreateConnection(); await undoLastMigration(connection); return; } else if (cmd === 'serve') { diff --git a/test/testUtils.ts b/test/testUtils.ts index cf9ee661..bda21d13 100644 --- a/test/testUtils.ts +++ b/test/testUtils.ts @@ -11,13 +11,5 @@ export async function getDatabase(typeormDb?: string): Promise { if (origTypeormDB) { process.env.TYPEORM_DATABASE = origTypeormDB; } - // If this is Sqlite, we are making a separate connection to the database, - // so could get busy errors. We bump up our timeout. The rest of Grist could - // get busy errors if we do slow writes though. - const connection = db.connection; - const sqlite = connection.driver.options.type === 'sqlite'; - if (sqlite) { - await db.connection.query('PRAGMA busy_timeout = 3000'); - } return db; }