Fix: await cache set (#446)

conico974 · web-flow · commit 8f1d2b42ab63 · 2024-06-19T13:35:11.000+02:00
* await cache set

* add new lint rules

* fix all linting issues

* Refactor detached promise handling

* fix e2e
remove lint from next build

* fix example and docs

* Create eight-parrots-peel.md
diff --git a/.changeset/eight-parrots-peel.md b/.changeset/eight-parrots-peel.md
@@ -0,0 +1,11 @@
+---
+"open-next-docs": patch
+"open-next-benchmark": patch
+"app-pages-router": patch
+"app-router": patch
+"pages-router": patch
+"open-next": patch
+"tests-e2e": patch
+---
+
+Fix: dangling promises
diff --git a/.eslintrc.cjs b/.eslintrc.cjs
@@ -16,5 +16,24 @@ module.exports = {
     "sonarjs/elseif-without-else": "warn",
     "sonarjs/no-duplicate-string": "warn",
     "sonarjs/cognitive-complexity": "warn",
+
+    // We add some typescript rules - The recommended rules breaks too much stuff
+    // TODO: We should add more rules, especially around typescript types
+
+    // Promises related rules
+    "@typescript-eslint/await-thenable": "error",
+    "@typescript-eslint/no-floating-promises": "error",
+    "@typescript-eslint/no-misused-promises": [
+      "error",
+      { checksVoidReturn: false },
+    ],
+
+    "@typescript-eslint/unbound-method": "error",
+
+    "@typescript-eslint/no-non-null-assertion": "warn",
+  },
+  parserOptions: {
+    project: ["./tsconfig.eslint.json", "./**/tsconfig.json"],
   },
+  ignorePatterns: ["**/node_modules/**", "**/dist/**", "**/out/**"],
 };
diff --git a/docs/next.config.js b/docs/next.config.js
@@ -34,6 +34,9 @@ const withNextra = require("nextra")({
 
 module.exports = withNextra({
   swcMinify: true,
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
   images: {
     unoptimized: true,
   },
diff --git a/example/next.config.js b/example/next.config.js
@@ -3,6 +3,9 @@ const nextConfig = {
   reactStrictMode: true,
   cleanDistDir: true,
   swcMinify: true,
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
   images: {
     remotePatterns: [
       {
diff --git a/examples/app-pages-router/next.config.js b/examples/app-pages-router/next.config.js
@@ -8,6 +8,9 @@ const nextConfig = {
   experimental: {
     serverActions: true,
   },
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
   trailingSlash: true,
   skipTrailingSlashRedirect: true,
 };
diff --git a/examples/app-router/app/api/sse/route.ts b/examples/app-router/app/api/sse/route.ts
@@ -16,15 +16,15 @@ export async function GET(request: NextRequest) {
   });
 
   setTimeout(async () => {
-    writer.write(
+    await writer.write(
       `data: ${JSON.stringify({
         message: "open",
         time: new Date().toISOString(),
       })}\n\n`,
     );
     for (let i = 1; i <= 4; i++) {
       await wait(2000);
-      writer.write(
+      await writer.write(
         `data: ${JSON.stringify({
           message: "hello:" + i,
           time: new Date().toISOString(),
@@ -33,7 +33,7 @@ export async function GET(request: NextRequest) {
     }
 
     await wait(2000); // Wait for 4 seconds
-    writer.write(
+    await writer.write(
       `data: ${JSON.stringify({
         message: "close",
         time: new Date().toISOString(),
diff --git a/examples/app-router/next.config.js b/examples/app-router/next.config.js
@@ -8,6 +8,9 @@ const nextConfig = {
   experimental: {
     serverActions: true,
   },
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
   images: {
     remotePatterns: [
       {
diff --git a/examples/pages-router/next.config.js b/examples/pages-router/next.config.js
@@ -5,6 +5,9 @@ const nextConfig = {
   reactStrictMode: true,
   output: "standalone",
   outputFileTracing: "../sst",
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
   rewrites: () => [
     { source: "/rewrite", destination: "/" },
     {
diff --git a/packages/open-next/src/adapters/cache.ts b/packages/open-next/src/adapters/cache.ts
@@ -1,5 +1,3 @@
-import { DetachedPromise } from "utils/promise.js";
-
 import { IncrementalCache } from "../cache/incremental/types.js";
 import { TagCache } from "../cache/tag/types.js";
 import { isBinaryContentType } from "./binary.js";
@@ -225,8 +223,11 @@ export default class S3Cache {
     if (globalThis.disableIncrementalCache) {
       return;
     }
-    const detachedPromise = new DetachedPromise<void>();
-    globalThis.__als.getStore()?.pendingPromises.push(detachedPromise);
+    // This one might not even be necessary anymore
+    // Better be safe than sorry
+    const detachedPromise = globalThis.__als
+      .getStore()
+      ?.pendingPromiseRunner.withResolvers<void>();
     try {
       if (data?.kind === "ROUTE") {
         const { body, status, headers } = data;
@@ -250,7 +251,7 @@ export default class S3Cache {
         const { html, pageData } = data;
         const isAppPath = typeof pageData === "string";
         if (isAppPath) {
-          globalThis.incrementalCache.set(
+          await globalThis.incrementalCache.set(
             key,
             {
               type: "app",
@@ -260,7 +261,7 @@ export default class S3Cache {
             false,
           );
         } else {
-          globalThis.incrementalCache.set(
+          await globalThis.incrementalCache.set(
             key,
             {
               type: "page",
@@ -312,7 +313,7 @@ export default class S3Cache {
       error("Failed to set cache", e);
     } finally {
       // We need to resolve the promise even if there was an error
-      detachedPromise.resolve();
+      detachedPromise?.resolve();
     }
   }
 
diff --git a/packages/open-next/src/build.ts b/packages/open-next/src/build.ts
@@ -76,7 +76,7 @@ export async function build(
   // Build Next.js app
   printHeader("Building Next.js app");
   setStandaloneBuildMode(monorepoRoot);
-  await buildNextjsApp(packager);
+  buildNextjsApp(packager);
 
   // Generate deployable bundle
   printHeader("Generating bundle");
@@ -280,7 +280,7 @@ async function createRevalidationBundle(config: OpenNextConfig) {
   copyOpenNextConfig(options.tempDir, outputPath);
 
   // Build Lambda code
-  esbuildAsync(
+  await esbuildAsync(
     {
       external: ["next", "styled-jsx", "react"],
       entryPoints: [path.join(__dirname, "adapters", "revalidate.js")],
diff --git a/packages/open-next/src/build/createServerBundle.ts b/packages/open-next/src/build/createServerBundle.ts
@@ -157,7 +157,7 @@ async function generateBundle(
   // Bundle next server if necessary
   const isBundled = fnOptions.experimentalBundledNextServer ?? false;
   if (isBundled) {
-    bundleNextServer(path.join(outputPath, packagePath), appPath);
+    await bundleNextServer(path.join(outputPath, packagePath), appPath);
   }
 
   // // Copy middleware
@@ -181,7 +181,7 @@ async function generateBundle(
   copyEnvFile(appBuildOutputPath, packagePath, outputPath);
 
   // Copy all necessary traced files
-  copyTracedFiles(
+  await copyTracedFiles(
     appBuildOutputPath,
     packagePath,
     outputPath,
diff --git a/packages/open-next/src/core/createMainHandler.ts b/packages/open-next/src/core/createMainHandler.ts
@@ -1,7 +1,7 @@
 import type { AsyncLocalStorage } from "node:async_hooks";
 
 import type { OpenNextConfig } from "types/open-next";
-import { DetachedPromise } from "utils/promise";
+import { DetachedPromiseRunner } from "utils/promise";
 
 import { debug } from "../adapters/logger";
 import { generateUniqueId } from "../adapters/util";
@@ -23,7 +23,7 @@ declare global {
   var serverId: string;
   var __als: AsyncLocalStorage<{
     requestId: string;
-    pendingPromises: DetachedPromise<void>[];
+    pendingPromiseRunner: DetachedPromiseRunner;
   }>;
 }
 
diff --git a/packages/open-next/src/core/requestHandler.ts b/packages/open-next/src/core/requestHandler.ts
@@ -6,7 +6,7 @@ import {
   StreamCreator,
 } from "http/index.js";
 import { InternalEvent, InternalResult } from "types/open-next";
-import { DetachedPromise } from "utils/promise";
+import { DetachedPromiseRunner } from "utils/promise";
 
 import { debug, error, warn } from "../adapters/logger";
 import { convertRes, createServerResponse, proxyRequest } from "./routing/util";
@@ -16,7 +16,7 @@ import { requestHandler, setNextjsPrebundledReact } from "./util";
 // This is used to identify requests in the cache
 globalThis.__als = new AsyncLocalStorage<{
   requestId: string;
-  pendingPromises: DetachedPromise<any>[];
+  pendingPromiseRunner: DetachedPromiseRunner;
 }>();
 
 export async function openNextHandler(
@@ -85,9 +85,10 @@ export async function openNextHandler(
       remoteAddress: preprocessedEvent.remoteAddress,
     };
     const requestId = Math.random().toString(36);
-    const pendingPromises: DetachedPromise<void>[] = [];
+    const pendingPromiseRunner: DetachedPromiseRunner =
+      new DetachedPromiseRunner();
     const internalResult = await globalThis.__als.run(
-      { requestId, pendingPromises },
+      { requestId, pendingPromiseRunner },
       async () => {
         const preprocessedResult = preprocessResult as MiddlewareOutputEvent;
         const req = new IncomingMessage(reqProps);
@@ -117,10 +118,7 @@ export async function openNextHandler(
         // reset lastModified. We need to do this to avoid memory leaks
         delete globalThis.lastModified[requestId];
 
-        // Wait for all promises to resolve
-        // We are not catching errors here, because they are catched before
-        // This may need to change in the future
-        await Promise.all(pendingPromises.map((p) => p.promise));
+        await pendingPromiseRunner.await();
 
         return internalResult;
       },
@@ -161,10 +159,10 @@ async function processRequest(
     if (e.constructor.name === "NoFallbackError") {
       // Do we need to handle _not-found
       // Ideally this should never get triggered and be intercepted by the routing handler
-      tryRenderError("404", res, internalEvent);
+      await tryRenderError("404", res, internalEvent);
     } else {
       error("NextJS request failed.", e);
-      tryRenderError("500", res, internalEvent);
+      await tryRenderError("500", res, internalEvent);
     }
   }
 }
diff --git a/packages/open-next/src/core/routing/util.ts b/packages/open-next/src/core/routing/util.ts
@@ -7,7 +7,6 @@ import { OpenNextNodeResponse } from "http/openNextResponse.js";
 import { parseHeaders } from "http/util.js";
 import type { MiddlewareManifest } from "types/next-types";
 import { InternalEvent } from "types/open-next.js";
-import { DetachedPromise } from "utils/promise.js";
 
 import { isBinaryContentType } from "../../adapters/binary.js";
 import { debug, error } from "../../adapters/logger.js";
@@ -356,11 +355,6 @@ export async function revalidateIfRequired(
         : internalMeta?._nextRewroteUrl
       : rawPath;
 
-    // We want to ensure that the revalidation is done in the background
-    // But we should still wait for the queue send to be successful
-    const detachedPromise = new DetachedPromise<void>();
-    globalThis.__als.getStore()?.pendingPromises.push(detachedPromise);
-
     // We need to pass etag to the revalidation queue to try to bypass the default 5 min deduplication window.
     // https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/using-messagededuplicationid-property.html
     // If you need to have a revalidation happen more frequently than 5 minutes,
@@ -387,9 +381,6 @@ export async function revalidateIfRequired(
       });
     } catch (e) {
       error(`Failed to revalidate stale page ${rawPath}`, e);
-    } finally {
-      // We don't care if it fails or not, we don't want to block the request
-      detachedPromise.resolve();
     }
   }
 }
diff --git a/packages/open-next/src/http/openNextResponse.ts b/packages/open-next/src/http/openNextResponse.ts
@@ -91,7 +91,9 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
       if (!this.headersSent) {
         this.flushHeaders();
       }
-      onEnd(this.headers);
+      globalThis.__als
+        .getStore()
+        ?.pendingPromiseRunner.add(onEnd(this.headers));
       const bodyLength = this.body.length;
       this.streamCreator?.onFinish(bodyLength);
     });
diff --git a/packages/open-next/src/index.ts b/packages/open-next/src/index.ts
@@ -8,7 +8,7 @@ if (command !== "build") printHelp();
 const args = parseArgs();
 if (Object.keys(args).includes("--help")) printHelp();
 
-build(args["--config-path"], args["--node-externals"]);
+await build(args["--config-path"], args["--node-externals"]);
 
 function parseArgs() {
   return process.argv.slice(2).reduce(
diff --git a/packages/open-next/src/utils/promise.ts b/packages/open-next/src/utils/promise.ts
@@ -1,3 +1,5 @@
+import { debug, error } from "../adapters/logger";
+
 /**
  * A `Promise.withResolvers` implementation that exposes the `resolve` and
  * `reject` functions on a `Promise`.
@@ -21,7 +23,38 @@ export class DetachedPromise<T = any> {
 
     // We know that resolvers is defined because the Promise constructor runs
     // synchronously.
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
     this.resolve = resolve!;
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
     this.reject = reject!;
   }
 }
+
+export class DetachedPromiseRunner {
+  private promises: DetachedPromise<any>[] = [];
+
+  public withResolvers<T>(): DetachedPromise<T> {
+    const detachedPromise = new DetachedPromise<T>();
+    this.promises.push(detachedPromise);
+    return detachedPromise;
+  }
+
+  public add<T>(promise: Promise<T>): void {
+    const detachedPromise = new DetachedPromise<T>();
+    this.promises.push(detachedPromise);
+    promise.then(detachedPromise.resolve, detachedPromise.reject);
+  }
+
+  public async await(): Promise<void> {
+    debug(`Awaiting ${this.promises.length} detached promises`);
+    const results = await Promise.allSettled(
+      this.promises.map((p) => p.promise),
+    );
+    const rejectedPromises = results.filter(
+      (r) => r.status === "rejected",
+    ) as PromiseRejectedResult[];
+    rejectedPromises.forEach((r) => {
+      error(r.reason);
+    });
+  }
+}
diff --git a/packages/tests-e2e/tests/appPagesRouter/pages_ssr.test.ts b/packages/tests-e2e/tests/appPagesRouter/pages_ssr.test.ts
@@ -21,7 +21,7 @@ test("Server Side Render", async ({ page }) => {
     el = page.getByText("Time:");
     newTime = await el.textContent();
     await expect(el).toBeVisible();
-    await expect(time).not.toEqual(newTime);
+    expect(time).not.toEqual(newTime);
     time = newTime;
     await wait(250);
   }
diff --git a/packages/tests-e2e/tests/appPagesRouter/ssr.test.ts b/packages/tests-e2e/tests/appPagesRouter/ssr.test.ts
@@ -21,7 +21,7 @@ test("Server Side Render", async ({ page }) => {
     el = page.getByText("Time:");
     newTime = await el.textContent();
     await expect(el).toBeVisible();
-    await expect(time).not.toEqual(newTime);
+    expect(time).not.toEqual(newTime);
     time = newTime;
     await wait(250);
   }
diff --git a/packages/tests-e2e/tests/appRouter/headers.test.ts b/packages/tests-e2e/tests/appRouter/headers.test.ts
@@ -12,10 +12,10 @@ test("Headers", async ({ page }) => {
   const response = await responsePromise;
   // Response header should be set
   const headers = response.headers();
-  await expect(headers["response-header"]).toEqual("response-header");
+  expect(headers["response-header"]).toEqual("response-header");
 
   // The next.config.js headers should be also set in response
-  await expect(headers["e2e-headers"]).toEqual("next.config.js");
+  expect(headers["e2e-headers"]).toEqual("next.config.js");
 
   // Request header should be available in RSC
   let el = page.getByText(`request-header`);
diff --git a/packages/tests-e2e/tests/appRouter/ssr.test.ts b/packages/tests-e2e/tests/appRouter/ssr.test.ts
diff --git a/packages/tests-e2e/tests/pagesRouter/ssr.test.ts b/packages/tests-e2e/tests/pagesRouter/ssr.test.ts
diff --git a/tsconfig.eslint.json b/tsconfig.eslint.json
