Revert "fix(preview-service): only create a puppeteer client per app,…

… not per…" (#2901) This reverts commit d5c9e5e.
specklesystems · Sep 7, 2024 · 19b51c5 · 19b51c5
1 parent 20fcc40
commit 19b51c5
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 82 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<E2ETestContext>({
       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()
       })