diff --git a/packages/preview-service/src/bin.ts b/packages/preview-service/src/bin.ts index eef23b07af..e93580a819 100644 --- a/packages/preview-service/src/bin.ts +++ b/packages/preview-service/src/bin.ts @@ -3,32 +3,6 @@ import '@/bootstrap.js' // This has side-effects and has to be imported first import { startServer } from '@/server/server.js' import { startPreviewService } from '@/server/background.js' import { db } from '@/clients/knex.js' -import { puppeteerClientFactory } from '@/clients/puppeteer.js' -import { extendLoggerComponent, logger } from '@/observability/logging.js' -import { puppeteerDriver } from '@/scripts/puppeteerDriver.js' -import { - getChromiumExecutablePath, - getPreviewTimeout, - getPuppeteerUserDataDir, - serviceOrigin, - shouldBeHeadless -} from '@/utils/env.js' -const puppeteerClient = await puppeteerClientFactory({ - logger: extendLoggerComponent(logger, 'puppeteerClient'), - url: `${serviceOrigin()}/render/`, - script: puppeteerDriver, - launchParams: { - headless: shouldBeHeadless(), - userDataDir: getPuppeteerUserDataDir(), - executablePath: getChromiumExecutablePath(), - protocolTimeout: getPreviewTimeout(), - // we trust the web content that is running, so can disable the sandbox - // disabling the sandbox allows us to run the docker image without linux kernel privileges - args: ['--no-sandbox', '--disable-setuid-sandbox', '--disable-dev-shm-usage'] - }, - timeoutMilliseconds: getPreviewTimeout() -}) - -startServer({ db, puppeteerClient }) +startServer({ db }) startPreviewService({ db }) diff --git a/packages/preview-service/src/server/app.ts b/packages/preview-service/src/server/app.ts index 564232dbff..42a8a684b9 100644 --- a/packages/preview-service/src/server/app.ts +++ b/packages/preview-service/src/server/app.ts @@ -1,4 +1,3 @@ -import { PuppeteerClient } from '@/clients/puppeteer.js' import { loggingExpressMiddleware } from '@/observability/expressLogging.js' import { srcRoot } from '@/root.js' import apiRouterFactory from '@/server/routes/api.js' @@ -11,8 +10,8 @@ import createError from 'http-errors' import type { Knex } from 'knex' import path from 'path' -export const appFactory = (deps: { db: Knex; puppeteerClient: PuppeteerClient }) => { - const { db, puppeteerClient } = deps +export const appFactory = (deps: { db: Knex }) => { + const { db } = deps const app = express() app.use(loggingExpressMiddleware) @@ -23,7 +22,7 @@ export const appFactory = (deps: { db: Knex; puppeteerClient: PuppeteerClient }) app.use(express.static(path.join(srcRoot, '../public'))) app.use('/', indexRouterFactory()) - app.use('/preview', previewRouterFactory({ puppeteerClient })) + app.use('/preview', previewRouterFactory()) app.use('/objects', objectsRouterFactory({ db })) app.use('/api', apiRouterFactory({ db })) diff --git a/packages/preview-service/src/server/routes/preview.ts b/packages/preview-service/src/server/routes/preview.ts index ea0125ba0a..c68490a3da 100644 --- a/packages/preview-service/src/server/routes/preview.ts +++ b/packages/preview-service/src/server/routes/preview.ts @@ -1,9 +1,16 @@ -import type { PuppeteerClient } from '@/clients/puppeteer.js' +import { puppeteerClientFactory } from '@/clients/puppeteer.js' +import { puppeteerDriver } from '@/scripts/puppeteerDriver.js' import { getScreenshotFactory } from '@/services/screenshot.js' -import { serviceOrigin } from '@/utils/env.js' -import express, { type RequestHandler } from 'express' +import { + getChromiumExecutablePath, + getPreviewTimeout, + getPuppeteerUserDataDir, + serviceOrigin, + shouldBeHeadless +} from '@/utils/env.js' +import express, { RequestHandler } from 'express' -const previewRouterFactory = (deps: { puppeteerClient: PuppeteerClient }) => { +const previewRouterFactory = () => { const previewRouter = express.Router() previewRouter.get( @@ -18,17 +25,37 @@ const previewRouterFactory = (deps: { puppeteerClient: PuppeteerClient }) => { boundLogger.info('Requesting screenshot.') - let screenshot: { [key: string]: string } | null = null - - screenshot = await getScreenshotFactory({ - loadPageAndEvaluateScript: deps.puppeteerClient.loadPageAndEvaluateScript, + //FIXME should we be creating a puppeteer client for every request, or per app instance? + const puppeteerClient = await puppeteerClientFactory({ logger: boundLogger, - serviceOrigin: serviceOrigin() - })({ - objectId, - streamId + url: `${serviceOrigin()}/render/`, + script: puppeteerDriver, + launchParams: { + headless: shouldBeHeadless(), + userDataDir: getPuppeteerUserDataDir(), + executablePath: getChromiumExecutablePath(), + protocolTimeout: getPreviewTimeout(), + // we trust the web content that is running, so can disable the sandbox + // disabling the sandbox allows us to run the docker image without linux kernel privileges + args: ['--no-sandbox', '--disable-setuid-sandbox', '--disable-dev-shm-usage'] + }, + timeoutMilliseconds: getPreviewTimeout() }) + let screenshot: { [key: string]: string } | null = null + try { + screenshot = await getScreenshotFactory({ + loadPageAndEvaluateScript: puppeteerClient.loadPageAndEvaluateScript, + logger: boundLogger, + serviceOrigin: serviceOrigin() + })({ + objectId, + streamId + }) + } finally { + await puppeteerClient.dispose() + } + if (!screenshot) { return res.status(500).end() } diff --git a/packages/preview-service/src/server/server.ts b/packages/preview-service/src/server/server.ts index 8ea4eb9afc..785786c481 100644 --- a/packages/preview-service/src/server/server.ts +++ b/packages/preview-service/src/server/server.ts @@ -5,20 +5,14 @@ import { getAppPort, getHost, getMetricsHost, getMetricsPort } from '@/utils/env import http from 'http' import type { Knex } from 'knex' import { isNaN, isString, toNumber } from 'lodash-es' -import type { PuppeteerClient } from '@/clients/puppeteer.js' - -export const startServer = (params: { - db: Knex - serveOnRandomPort?: boolean - puppeteerClient: PuppeteerClient -}) => { - const { db, serveOnRandomPort, puppeteerClient } = params +export const startServer = (params: { db: Knex; serveOnRandomPort?: boolean }) => { + const { db } = params /** * Get port from environment and store in Express. */ - const inputPort = serveOnRandomPort ? 0 : normalizePort(getAppPort()) - const app = appFactory({ db, puppeteerClient }) + const inputPort = params.serveOnRandomPort ? 0 : normalizePort(getAppPort()) + const app = appFactory({ db }) app.set('port', inputPort) // we place the metrics on a separate port as we wish to expose it to external monitoring tools, but do not wish to expose other routes (for now) @@ -57,12 +51,8 @@ export const startServer = (params: { return { app, server, metricsServer } } -export const stopServer = async (params: { - server: http.Server - puppeteerClient: PuppeteerClient -}) => { - const { server, puppeteerClient } = params - await puppeteerClient.dispose() +export const stopServer = (params: { server: http.Server }) => { + const { server } = params server.close() } diff --git a/packages/preview-service/tests/helpers/helpers.ts b/packages/preview-service/tests/helpers/helpers.ts index c1321909ea..c97b10855a 100644 --- a/packages/preview-service/tests/helpers/helpers.ts +++ b/packages/preview-service/tests/helpers/helpers.ts @@ -3,21 +3,13 @@ import type { Knex } from 'knex' import http from 'http' import type { AddressInfo } from 'net' import { getPostgresConnectionString } from '@/utils/env.js' -import { PuppeteerClient } from '@/clients/puppeteer.js' -export const startAndWaitOnServers = async (deps: { - db: Knex - puppeteerClient: PuppeteerClient -}) => { +export const startAndWaitOnServers = async (deps: { db: Knex }) => { let serverAddress: string | AddressInfo | null = null let metricsServerAddress: string | AddressInfo | null = null - const { db, puppeteerClient } = deps - const { app, server, metricsServer } = startServer({ - db, - puppeteerClient, - serveOnRandomPort: true - }) + const { db } = deps + const { app, server, metricsServer } = startServer({ db, serveOnRandomPort: true }) server.on('listening', () => { serverAddress = server.address() }) diff --git a/packages/preview-service/tests/helpers/testExtensions.ts b/packages/preview-service/tests/helpers/testExtensions.ts index 70cf2c636b..3c8c48d5e1 100644 --- a/packages/preview-service/tests/helpers/testExtensions.ts +++ b/packages/preview-service/tests/helpers/testExtensions.ts @@ -4,7 +4,6 @@ import { getTestDb } from '#/helpers/testKnexClient.js' import { startAndWaitOnServers } from '#/helpers/helpers.js' import type { Knex } from 'knex' import { Server } from 'http' -import { PuppeteerClient } from '@/clients/puppeteer.js' export interface AcceptanceTestContext { context: { @@ -79,20 +78,12 @@ export const e2eTest = test.extend({ const dbName = inject('dbName') // equivalent of beforeEach const db = await getTestDb(dbName).transaction() - const puppeteerClient: PuppeteerClient = { - //FIXME this is a stub for the moment - loadPageAndEvaluateScript: async () => {}, - dispose: async () => {} - } - const { server, metricsServer } = await startAndWaitOnServers({ - db, - puppeteerClient - }) + const { server, metricsServer } = await startAndWaitOnServers({ db }) // schedule the cleanup. Runs regardless of test status, and runs after afterEach. onTestFinished(async () => { - if (server) await stopServer({ server, puppeteerClient }) - if (metricsServer) await stopServer({ server: metricsServer, puppeteerClient }) + if (server) stopServer({ server }) + if (metricsServer) stopServer({ server: metricsServer }) if (db) await db.rollback() })