From 7c7bd4d5cba5f033c6f00362bda65092b9b6e1fc Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 30 May 2023 17:41:20 +0100 Subject: [PATCH 1/3] Fix for debugging with webstorm the old way (if desired), updating the builder middleware to be more multi-dev capable, ignoring 409s when attempting to update the last updated at for apps (if multiple devs hit at same time, only use one) also updating writethrough cache to retry once, with the extended TTL on locks, plus the multi-dev collab it can take a minute to update usage quota doc when a lot of updates occur at once. --- .../backend-core/src/cache/writethrough.ts | 2 +- .../backend-core/src/redis/redlockImpl.ts | 38 ++++++++++++------- packages/server/src/middleware/builder.ts | 35 ++++++++++------- .../server/src/utilities/fileSystem/app.ts | 18 +++++++-- packages/types/src/sdk/locks.ts | 1 + 5 files changed, 62 insertions(+), 32 deletions(-) diff --git a/packages/backend-core/src/cache/writethrough.ts b/packages/backend-core/src/cache/writethrough.ts index e64c116663..d399b17896 100644 --- a/packages/backend-core/src/cache/writethrough.ts +++ b/packages/backend-core/src/cache/writethrough.ts @@ -44,7 +44,7 @@ async function put( if (updateDb) { const lockResponse = await locks.doWithLock( { - type: LockType.TRY_ONCE, + type: LockType.TRY_TWICE, name: LockName.PERSIST_WRITETHROUGH, resource: key, ttl: 15000, diff --git a/packages/backend-core/src/redis/redlockImpl.ts b/packages/backend-core/src/redis/redlockImpl.ts index 55b891ea84..0698335b7c 100644 --- a/packages/backend-core/src/redis/redlockImpl.ts +++ b/packages/backend-core/src/redis/redlockImpl.ts @@ -4,10 +4,10 @@ import { LockOptions, LockType } from "@budibase/types" import * as context from "../context" import env from "../environment" -const getClient = async ( +async function getClient( type: LockType, opts?: Redlock.Options -): Promise => { +): Promise { if (type === LockType.CUSTOM) { return newRedlock(opts) } @@ -18,6 +18,9 @@ const getClient = async ( case LockType.TRY_ONCE: { return newRedlock(OPTIONS.TRY_ONCE) } + case LockType.TRY_TWICE: { + return newRedlock(OPTIONS.TRY_TWICE) + } case LockType.DEFAULT: { return newRedlock(OPTIONS.DEFAULT) } @@ -35,6 +38,9 @@ const OPTIONS = { // immediately throws an error if the lock is already held retryCount: 0, }, + TRY_TWICE: { + retryCount: 1, + }, TEST: { // higher retry count in unit tests // due to high contention. @@ -62,7 +68,7 @@ const OPTIONS = { }, } -const newRedlock = async (opts: Redlock.Options = {}) => { +export async function newRedlock(opts: Redlock.Options = {}) { let options = { ...OPTIONS.DEFAULT, ...opts } const redisWrapper = await getLockClient() const client = redisWrapper.getClient() @@ -81,22 +87,26 @@ type RedlockExecution = | SuccessfulRedlockExecution | UnsuccessfulRedlockExecution -export const doWithLock = async ( +function getLockName(opts: LockOptions) { + // determine lock name + // by default use the tenantId for uniqueness, unless using a system lock + const prefix = opts.systemLock ? "system" : context.getTenantId() + let name: string = `lock:${prefix}_${opts.name}` + // add additional unique name if required + if (opts.resource) { + name = name + `_${opts.resource}` + } + return name +} + +export async function doWithLock( opts: LockOptions, task: () => Promise -): Promise> => { +): Promise> { const redlock = await getClient(opts.type, opts.customOptions) let lock try { - // determine lock name - // by default use the tenantId for uniqueness, unless using a system lock - const prefix = opts.systemLock ? "system" : context.getTenantId() - let name: string = `lock:${prefix}_${opts.name}` - - // add additional unique name if required - if (opts.resource) { - name = name + `_${opts.resource}` - } + const name = getLockName(opts) // create the lock lock = await redlock.lock(name, opts.ttl) diff --git a/packages/server/src/middleware/builder.ts b/packages/server/src/middleware/builder.ts index ffb2e2c002..31c4da127c 100644 --- a/packages/server/src/middleware/builder.ts +++ b/packages/server/src/middleware/builder.ts @@ -9,8 +9,8 @@ import { checkDebounce, setDebounce, } from "../utilities/redis" -import { db as dbCore, cache, permissions } from "@budibase/backend-core" -import { BBContext, Database } from "@budibase/types" +import { db as dbCore, cache } from "@budibase/backend-core" +import { UserCtx, Database } from "@budibase/types" const DEBOUNCE_TIME_SEC = 30 @@ -23,7 +23,7 @@ const DEBOUNCE_TIME_SEC = 30 * through the authorized middleware * ****************************************************/ -async function checkDevAppLocks(ctx: BBContext) { +async function checkDevAppLocks(ctx: UserCtx) { const appId = ctx.appId // if any public usage, don't proceed @@ -42,7 +42,7 @@ async function checkDevAppLocks(ctx: BBContext) { } } -async function updateAppUpdatedAt(ctx: BBContext) { +async function updateAppUpdatedAt(ctx: UserCtx) { const appId = ctx.appId // if debouncing skip this update // get methods also aren't updating @@ -50,20 +50,29 @@ async function updateAppUpdatedAt(ctx: BBContext) { return } await dbCore.doWithDB(appId, async (db: Database) => { - const metadata = await db.get(DocumentType.APP_METADATA) - metadata.updatedAt = new Date().toISOString() + try { + const metadata = await db.get(DocumentType.APP_METADATA) + metadata.updatedAt = new Date().toISOString() - metadata.updatedBy = getGlobalIDFromUserMetadataID(ctx.user?.userId!) + metadata.updatedBy = getGlobalIDFromUserMetadataID(ctx.user?.userId!) - const response = await db.put(metadata) - metadata._rev = response.rev - await cache.app.invalidateAppMetadata(appId, metadata) - // set a new debounce record with a short TTL - await setDebounce(appId, DEBOUNCE_TIME_SEC) + const response = await db.put(metadata) + metadata._rev = response.rev + await cache.app.invalidateAppMetadata(appId, metadata) + // set a new debounce record with a short TTL + await setDebounce(appId, DEBOUNCE_TIME_SEC) + } catch (err: any) { + // if a 409 occurs, then multiple clients connected at the same time - ignore + if (err?.status === 409) { + return + } else { + throw err + } + } }) } -export default async function builder(ctx: BBContext) { +export default async function builder(ctx: UserCtx) { const appId = ctx.appId // this only functions within an app context if (!appId) { diff --git a/packages/server/src/utilities/fileSystem/app.ts b/packages/server/src/utilities/fileSystem/app.ts index 25600ee3f1..16681c2978 100644 --- a/packages/server/src/utilities/fileSystem/app.ts +++ b/packages/server/src/utilities/fileSystem/app.ts @@ -35,10 +35,20 @@ export const getComponentLibraryManifest = async (library: string) => { const filename = "manifest.json" if (env.isDev() || env.isTest()) { - const path = join(TOP_LEVEL_PATH, "packages/client", filename) - // always load from new so that updates are refreshed - delete require.cache[require.resolve(path)] - return require(path) + const paths = [ + join(TOP_LEVEL_PATH, "packages/client", filename), + join(process.cwd(), "client", filename), + ] + for (let path of paths) { + if (fs.existsSync(path)) { + // always load from new so that updates are refreshed + delete require.cache[require.resolve(path)] + return require(path) + } + } + throw new Error( + `Unable to find ${filename} in development environment (may need to build).` + ) } if (!appId) { diff --git a/packages/types/src/sdk/locks.ts b/packages/types/src/sdk/locks.ts index 6147308f7d..a35e7b379b 100644 --- a/packages/types/src/sdk/locks.ts +++ b/packages/types/src/sdk/locks.ts @@ -6,6 +6,7 @@ export enum LockType { * No retries will take place and no error will be thrown. */ TRY_ONCE = "try_once", + TRY_TWICE = "try_twice", DEFAULT = "default", DELAY_500 = "delay_500", CUSTOM = "custom", From 5249148d6cf4c5f686288a3085e01eed90e8e769 Mon Sep 17 00:00:00 2001 From: Michael Drury Date: Tue, 30 May 2023 20:20:22 +0100 Subject: [PATCH 2/3] Updating writethrough test to be aware of the double attempt locks. --- .../src/cache/tests/writethrough.spec.ts | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/packages/backend-core/src/cache/tests/writethrough.spec.ts b/packages/backend-core/src/cache/tests/writethrough.spec.ts index e4c7cc6e64..d3125537a8 100644 --- a/packages/backend-core/src/cache/tests/writethrough.spec.ts +++ b/packages/backend-core/src/cache/tests/writethrough.spec.ts @@ -72,20 +72,26 @@ describe("writethrough", () => { writethrough.put({ ...current, value: 4 }), ]) - const newRev = responses.map(x => x.rev).find(x => x !== current._rev) - expect(newRev).toBeDefined() - expect(responses.map(x => x.rev)).toEqual( - expect.arrayContaining([current._rev, current._rev, newRev]) - ) - expectFunctionWasCalledTimesWith( - mocks.alerts.logWarn, - 2, - "Ignoring redlock conflict in write-through cache" - ) + // with a lock, this will work + const revs = responses.map(x => x.rev) + const startWith = ["3", "4", "5"] + const found = [] + let maxRev + for (let starting of startWith) { + for (let rev of revs) { + if (rev?.startsWith(starting)) { + found.push(starting) + } + if (rev?.startsWith("5")) { + maxRev = rev + } + } + } + expect(found.length).toBe(3) const output = await db.get(current._id) expect(output.value).toBe(4) - expect(output._rev).toBe(newRev) + expect(output._rev).toBe(maxRev) current = output }) From 99607ca06e058205d80b2186ae04154f2175369e Mon Sep 17 00:00:00 2001 From: Michael Drury Date: Tue, 30 May 2023 20:45:10 +0100 Subject: [PATCH 3/3] Reverting try twice change to writethrough. --- .../src/cache/tests/writethrough.spec.ts | 22 +++++-------------- .../backend-core/src/cache/writethrough.ts | 2 +- .../backend-core/src/redis/redlockImpl.ts | 1 - 3 files changed, 7 insertions(+), 18 deletions(-) diff --git a/packages/backend-core/src/cache/tests/writethrough.spec.ts b/packages/backend-core/src/cache/tests/writethrough.spec.ts index d3125537a8..92b073ed64 100644 --- a/packages/backend-core/src/cache/tests/writethrough.spec.ts +++ b/packages/backend-core/src/cache/tests/writethrough.spec.ts @@ -73,25 +73,15 @@ describe("writethrough", () => { ]) // with a lock, this will work - const revs = responses.map(x => x.rev) - const startWith = ["3", "4", "5"] - const found = [] - let maxRev - for (let starting of startWith) { - for (let rev of revs) { - if (rev?.startsWith(starting)) { - found.push(starting) - } - if (rev?.startsWith("5")) { - maxRev = rev - } - } - } - expect(found.length).toBe(3) + const newRev = responses.map(x => x.rev).find(x => x !== current._rev) + expect(newRev).toBeDefined() + expect(responses.map(x => x.rev)).toEqual( + expect.arrayContaining([current._rev, current._rev, newRev]) + ) const output = await db.get(current._id) expect(output.value).toBe(4) - expect(output._rev).toBe(maxRev) + expect(output._rev).toBe(newRev) current = output }) diff --git a/packages/backend-core/src/cache/writethrough.ts b/packages/backend-core/src/cache/writethrough.ts index d399b17896..e64c116663 100644 --- a/packages/backend-core/src/cache/writethrough.ts +++ b/packages/backend-core/src/cache/writethrough.ts @@ -44,7 +44,7 @@ async function put( if (updateDb) { const lockResponse = await locks.doWithLock( { - type: LockType.TRY_TWICE, + type: LockType.TRY_ONCE, name: LockName.PERSIST_WRITETHROUGH, resource: key, ttl: 15000, diff --git a/packages/backend-core/src/redis/redlockImpl.ts b/packages/backend-core/src/redis/redlockImpl.ts index 0698335b7c..7fe61a409e 100644 --- a/packages/backend-core/src/redis/redlockImpl.ts +++ b/packages/backend-core/src/redis/redlockImpl.ts @@ -122,7 +122,6 @@ export async function doWithLock( if (opts.type === LockType.TRY_ONCE) { // don't throw for try-once locks, they will always error // due to retry count (0) exceeded - console.warn(e) return { executed: false } } else { console.error(e)