From ea8791c69df931c9c8c7482570dff9f0078cc423 Mon Sep 17 00:00:00 2001
From: Matchu <matchu1993@gmail.com>
Date: Thu, 2 Sep 2021 19:25:48 -0700
Subject: [PATCH] Create a new browser for each assetImage request
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Well, holding onto the browser instance seems to be a source of bugs (my previous fix didn't seem to fix it), and I'm not _sure_ what the perf characteristics are, so let's just try a fresh instance each time!

I don't actually know what a browser "instance" in Playwright really is, I'm not sure it even necessarily creates a new process, I just don't know

and I saw some Vercel example code take this approach, which is definitely simpler, and I guess must not be _overtly_ bad perf if it's idiomatic?

So, like, ok, cool, let's see if this stops 500ing us with "Browser closed"! 😅
---
 api/assetImage.js | 46 +++++++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/api/assetImage.js b/api/assetImage.js
index b71ad9f..97d8ebb 100644
--- a/api/assetImage.js
+++ b/api/assetImage.js
@@ -30,37 +30,25 @@ const ASSET_IMAGE_PAGE_BASE_URL = process.env.VERCEL_URL
   ? "http://localhost:3000/internal/assetImage"
   : "https://impress-2020.openneo.net/internal/assetImage";
 
-// TODO: What are the perf implications of sharing one browser instance, with
-//       multiple pages open? This feels optimal to me from the *obvious*
-//       perspective, but I do wonder whether e.g. there are surprise
-//       implications from sharing a browser instance, or if too many pages in
-//       parallel will be a problem for our API endpoint.
-let BROWSER;
+// TODO: We used to share a browser instamce, but we couldn't get it to reload
+//       correctly after accidental closes, so we're just gonna always load a
+//       new one now. What are the perf implications of this? Does it slow down
+//       response time substantially?
 async function getBrowser() {
-  // Sometimes, the browser crashes. Maybe a RAM thing? If that happened, set
-  // it to null, and then we'll replace it with a new instance below.
-  if (BROWSER && !BROWSER.isConnected()) {
-    console.info("Browser is no longer connected; rebooting.");
-    BROWSER = null;
+  if (process.env["NODE_ENV"] === "production") {
+    // In production, we use a special chrome-aws-lambda Chromium.
+    const chromium = require("chrome-aws-lambda");
+    const playwright = require("playwright-core");
+    return await playwright.chromium.launch({
+      args: chromium.args,
+      executablePath: await chromium.executablePath,
+      headless: true,
+    });
+  } else {
+    // In development, we use the standard playwright Chromium.
+    const playwright = require("playwright");
+    return await playwright.chromium.launch({ headless: true });
   }
-
-  if (!BROWSER) {
-    if (process.env["NODE_ENV"] === "production") {
-      // In production, we use a special chrome-aws-lambda Chromium.
-      const chromium = require("chrome-aws-lambda");
-      const playwright = require("playwright-core");
-      BROWSER = await playwright.chromium.launch({
-        args: chromium.args,
-        executablePath: await chromium.executablePath,
-        headless: true,
-      });
-    } else {
-      // In development, we use the standard playwright Chromium.
-      const playwright = require("playwright");
-      BROWSER = await playwright.chromium.launch({ headless: true });
-    }
-  }
-  return BROWSER;
 }
 
 async function handle(req, res) {