From ba002f96492fd311cb0396262b4f77c2edb6f8f5 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 2 Feb 2024 09:30:33 +0000 Subject: [PATCH 1/7] Clean up isolates when a request is finished. --- packages/backend-core/src/context/types.ts | 1 + packages/server/src/jsRunner/index.ts | 8 ++++++++ packages/server/src/middleware/cleanup.ts | 16 ++++++++++++++++ 3 files changed, 25 insertions(+) create mode 100644 packages/server/src/middleware/cleanup.ts diff --git a/packages/backend-core/src/context/types.ts b/packages/backend-core/src/context/types.ts index cc052ca505..dad1af3bf8 100644 --- a/packages/backend-core/src/context/types.ts +++ b/packages/backend-core/src/context/types.ts @@ -15,4 +15,5 @@ export type ContextMap = { jsContext: Context helpersModule: Module } + cleanup?: (() => void | Promise)[] } diff --git a/packages/server/src/jsRunner/index.ts b/packages/server/src/jsRunner/index.ts index 8e529d533d..9441a74d07 100644 --- a/packages/server/src/jsRunner/index.ts +++ b/packages/server/src/jsRunner/index.ts @@ -115,6 +115,14 @@ export function init() { } bbCtx.isolateRefs = { jsContext, jsIsolate, helpersModule } + if (!bbCtx.cleanup) { + bbCtx.cleanup = [] + } + bbCtx.cleanup.push(() => { + helpersModule.release() + jsContext.release() + jsIsolate.dispose() + }) } let { jsIsolate, jsContext, helpersModule } = bbCtx.isolateRefs! diff --git a/packages/server/src/middleware/cleanup.ts b/packages/server/src/middleware/cleanup.ts new file mode 100644 index 0000000000..5204d5cfb1 --- /dev/null +++ b/packages/server/src/middleware/cleanup.ts @@ -0,0 +1,16 @@ +import { Ctx } from "@budibase/types" +import { context } from "@budibase/backend-core" + +export default async (ctx: Ctx, next: any) => { + const resp = next() + + const current = context.getCurrentContext() + if (current?.cleanup) { + for (let fn of current.cleanup || []) { + await fn() + } + delete current.cleanup + } + + return resp +} From 21dfbe75ffa19c4be85d4a8fb785f028a47a8aa3 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 2 Feb 2024 09:32:07 +0000 Subject: [PATCH 2/7] Use new cleanup middleware. --- packages/server/src/api/index.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/server/src/api/index.ts b/packages/server/src/api/index.ts index ad3d8307da..92cee95ea6 100644 --- a/packages/server/src/api/index.ts +++ b/packages/server/src/api/index.ts @@ -1,6 +1,7 @@ import Router from "@koa/router" import { auth, middleware, env as envCore } from "@budibase/backend-core" import currentApp from "../middleware/currentapp" +import cleanup from "../middleware/cleanup" import zlib from "zlib" import { mainRoutes, staticRoutes, publicRoutes } from "./routes" import { middleware as pro } from "@budibase/pro" @@ -62,6 +63,8 @@ if (apiEnabled()) { .use(auth.auditLog) // @ts-ignore .use(migrations) + // @ts-ignore + .use(cleanup) // authenticated routes for (let route of mainRoutes) { From a3efab01bf6ffb2b5af71486732f39dcc8f0ac79 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 2 Feb 2024 14:57:05 +0000 Subject: [PATCH 3/7] Update packages/server/src/middleware/cleanup.ts Co-authored-by: Adria Navarro --- packages/server/src/middleware/cleanup.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/middleware/cleanup.ts b/packages/server/src/middleware/cleanup.ts index 5204d5cfb1..d59317feed 100644 --- a/packages/server/src/middleware/cleanup.ts +++ b/packages/server/src/middleware/cleanup.ts @@ -2,7 +2,7 @@ import { Ctx } from "@budibase/types" import { context } from "@budibase/backend-core" export default async (ctx: Ctx, next: any) => { - const resp = next() + const resp = await next() const current = context.getCurrentContext() if (current?.cleanup) { From b8b12ff9393a88d4f012e990d6093c7141c3d9b4 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 21 Feb 2024 15:26:26 +0000 Subject: [PATCH 4/7] Respond to PR feedback. --- packages/account-portal | 2 +- packages/pro | 2 +- packages/server/src/jsRunner/index.ts | 29 +++++++++++++---------- packages/server/src/middleware/cleanup.ts | 22 +++++++++++++---- 4 files changed, 37 insertions(+), 18 deletions(-) diff --git a/packages/account-portal b/packages/account-portal index 52f51dcfb9..4de0d98e2f 160000 --- a/packages/account-portal +++ b/packages/account-portal @@ -1 +1 @@ -Subproject commit 52f51dcfb96d3fe58c8cc7a905e7d733f7cd84c2 +Subproject commit 4de0d98e2f8d80ee7631dffe076063273812a441 diff --git a/packages/pro b/packages/pro index 4f9616f163..60e47a8249 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit 4f9616f163039a0eea81319d8e2288340a2ebc79 +Subproject commit 60e47a8249fd6291a6bc20fe3fe6776b11938fa1 diff --git a/packages/server/src/jsRunner/index.ts b/packages/server/src/jsRunner/index.ts index f6a7f07cd1..3a2c0ac1a2 100644 --- a/packages/server/src/jsRunner/index.ts +++ b/packages/server/src/jsRunner/index.ts @@ -9,6 +9,7 @@ import { context, logging } from "@budibase/backend-core" import tracer from "dd-trace" import { IsolatedVM } from "./vm" +import type { VM } from "@budibase/types" export function init() { setJSRunner((js: string, ctx: Record) => { @@ -16,22 +17,26 @@ export function init() { try { const bbCtx = context.getCurrentContext() - const vm = bbCtx?.vm - ? bbCtx.vm - : new IsolatedVM({ - memoryLimit: env.JS_RUNNER_MEMORY_LIMIT, - invocationTimeout: env.JS_PER_INVOCATION_TIMEOUT_MS, - isolateAccumulatedTimeout: env.JS_PER_REQUEST_TIMEOUT_MS, - }).withHelpers() + let vm: VM + if (bbCtx && bbCtx.vm) { + vm = bbCtx.vm + } else { + vm = new IsolatedVM({ + memoryLimit: env.JS_RUNNER_MEMORY_LIMIT, + invocationTimeout: env.JS_PER_INVOCATION_TIMEOUT_MS, + isolateAccumulatedTimeout: env.JS_PER_REQUEST_TIMEOUT_MS, + }).withHelpers() + } - if (bbCtx) { - // If we have a context, we want to persist it to reuse the isolate + if (bbCtx && !bbCtx.vm) { bbCtx.vm = vm bbCtx.cleanup = bbCtx.cleanup || [] - bbCtx.cleanup.push(() => { - vm.close() - }) + bbCtx.cleanup.push(() => vm.close()) } + + // Because we can't pass functions into an Isolate, we remove them from + // the passed context and rely on the withHelpers() method to add them + // back in. const { helpers, ...rest } = ctx return vm.withContext(rest, () => vm.execute(js)) } catch (error: any) { diff --git a/packages/server/src/middleware/cleanup.ts b/packages/server/src/middleware/cleanup.ts index 5204d5cfb1..a810879a65 100644 --- a/packages/server/src/middleware/cleanup.ts +++ b/packages/server/src/middleware/cleanup.ts @@ -2,14 +2,28 @@ import { Ctx } from "@budibase/types" import { context } from "@budibase/backend-core" export default async (ctx: Ctx, next: any) => { - const resp = next() + const resp = await next() const current = context.getCurrentContext() - if (current?.cleanup) { - for (let fn of current.cleanup || []) { + if (!current || !current.cleanup) { + return resp + } + + let errors = [] + for (let fn of current.cleanup) { + try { await fn() + } catch (e) { + // We catch errors here to ensure we at least attempt to run all cleanup + // functions. We'll throw the first error we encounter after all cleanup + // functions have been run. + errors.push(e) } - delete current.cleanup + } + delete current.cleanup + + if (errors.length > 0) { + throw errors[0] } return resp From 6157e1becf9d89a6f5c525466d861b709eaf4c88 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 6 Mar 2024 14:55:59 +0000 Subject: [PATCH 5/7] Update pro submodule. --- packages/pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pro b/packages/pro index 60e47a8249..22a278da72 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit 60e47a8249fd6291a6bc20fe3fe6776b11938fa1 +Subproject commit 22a278da720d92991dabdcd4cb6c96e7abe29781 From ce599e775f001342526ead7c0df0c00d2b93abfd Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 7 Mar 2024 14:56:30 +0000 Subject: [PATCH 6/7] Add APM spans for request cleanup functions. --- packages/server/src/middleware/cleanup.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/server/src/middleware/cleanup.ts b/packages/server/src/middleware/cleanup.ts index a810879a65..43f945ab6b 100644 --- a/packages/server/src/middleware/cleanup.ts +++ b/packages/server/src/middleware/cleanup.ts @@ -1,5 +1,6 @@ import { Ctx } from "@budibase/types" import { context } from "@budibase/backend-core" +import { tracer } from "dd-trace" export default async (ctx: Ctx, next: any) => { const resp = await next() @@ -12,7 +13,9 @@ export default async (ctx: Ctx, next: any) => { let errors = [] for (let fn of current.cleanup) { try { - await fn() + await tracer.trace("cleanup", async span => { + await fn() + }) } catch (e) { // We catch errors here to ensure we at least attempt to run all cleanup // functions. We'll throw the first error we encounter after all cleanup From 9b91e47220304fc7e17b53ac842b8c2e3e710bd2 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 7 Mar 2024 15:01:38 +0000 Subject: [PATCH 7/7] Respond to Adri's feedback. --- packages/server/src/jsRunner/index.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/server/src/jsRunner/index.ts b/packages/server/src/jsRunner/index.ts index 61821c0d0e..19bf0fa6b5 100644 --- a/packages/server/src/jsRunner/index.ts +++ b/packages/server/src/jsRunner/index.ts @@ -16,16 +16,13 @@ export function init() { try { const bbCtx = context.getCurrentContext() - let vm: VM - if (bbCtx && bbCtx.vm) { - vm = bbCtx.vm - } else { - vm = new IsolatedVM({ + const vm = + bbCtx?.vm || + new IsolatedVM({ memoryLimit: env.JS_RUNNER_MEMORY_LIMIT, invocationTimeout: env.JS_PER_INVOCATION_TIMEOUT_MS, isolateAccumulatedTimeout: env.JS_PER_REQUEST_TIMEOUT_MS, }).withHelpers() - } if (bbCtx && !bbCtx.vm) { bbCtx.vm = vm