From 8f1d2b42ab630217e7069cdf087414a25704227c Mon Sep 17 00:00:00 2001 From: conico974 Date: Wed, 19 Jun 2024 13:35:11 +0200 Subject: [PATCH] Fix: await cache set (#446) * 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 --- .changeset/eight-parrots-peel.md | 11 +++++++ .eslintrc.cjs | 19 +++++++++++ docs/next.config.js | 3 ++ example/next.config.js | 3 ++ examples/app-pages-router/next.config.js | 3 ++ examples/app-router/app/api/sse/route.ts | 6 ++-- examples/app-router/next.config.js | 3 ++ examples/pages-router/next.config.js | 3 ++ packages/open-next/src/adapters/cache.ts | 15 +++++---- packages/open-next/src/build.ts | 4 +-- .../open-next/src/build/createServerBundle.ts | 4 +-- .../open-next/src/core/createMainHandler.ts | 4 +-- packages/open-next/src/core/requestHandler.ts | 18 +++++----- packages/open-next/src/core/routing/util.ts | 9 ----- .../open-next/src/http/openNextResponse.ts | 4 ++- packages/open-next/src/index.ts | 2 +- packages/open-next/src/utils/promise.ts | 33 +++++++++++++++++++ .../tests/appPagesRouter/pages_ssr.test.ts | 2 +- .../tests/appPagesRouter/ssr.test.ts | 2 +- .../tests-e2e/tests/appRouter/headers.test.ts | 4 +-- .../tests-e2e/tests/appRouter/ssr.test.ts | 2 +- .../tests-e2e/tests/pagesRouter/ssr.test.ts | 2 +- tsconfig.eslint.json | 19 +++++++++++ 23 files changed, 132 insertions(+), 43 deletions(-) create mode 100644 .changeset/eight-parrots-peel.md create mode 100644 tsconfig.eslint.json diff --git a/.changeset/eight-parrots-peel.md b/.changeset/eight-parrots-peel.md new file mode 100644 index 00000000..f7107ff3 --- /dev/null +++ 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 index b5cb971a..e75e2347 100644 --- 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 index 7a885ec3..0279b73f 100644 --- 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 index f6ea2fc1..079d52c9 100644 --- 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 index fe8b00c8..81700cec 100644 --- 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 index 375af959..dc1a705c 100644 --- a/examples/app-router/app/api/sse/route.ts +++ b/examples/app-router/app/api/sse/route.ts @@ -16,7 +16,7 @@ export async function GET(request: NextRequest) { }); setTimeout(async () => { - writer.write( + await writer.write( `data: ${JSON.stringify({ message: "open", time: new Date().toISOString(), @@ -24,7 +24,7 @@ export async function GET(request: NextRequest) { ); 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 index ce0565a8..d35b9fc2 100644 --- 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 index 66c213a2..a0a5044b 100644 --- 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 index f814a2d1..d689b17b 100644 --- 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(); - 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(); 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 index 4afc8c62..0fcbe7b1 100755 --- 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 index 1042a157..af223502 100644 --- 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 index 6d171b7f..67c7dd20 100644 --- 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[]; + pendingPromiseRunner: DetachedPromiseRunner; }>; } diff --git a/packages/open-next/src/core/requestHandler.ts b/packages/open-next/src/core/requestHandler.ts index b61c3508..18e82a4d 100644 --- 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[]; + 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[] = []; + 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 index 292b7ab2..d2bcb88d 100644 --- 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(); - 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 index f4e67158..bdaeefe5 100644 --- 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 index 2172553e..f8e933cb 100755 --- 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 index dffb609f..d18d3410 100644 --- 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 { // 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[] = []; + + public withResolvers(): DetachedPromise { + const detachedPromise = new DetachedPromise(); + this.promises.push(detachedPromise); + return detachedPromise; + } + + public add(promise: Promise): void { + const detachedPromise = new DetachedPromise(); + this.promises.push(detachedPromise); + promise.then(detachedPromise.resolve, detachedPromise.reject); + } + + public async await(): Promise { + 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 index d1c9ef73..2c013e04 100644 --- 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 index 0b907b0f..b271e3d1 100644 --- 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 index bc81ed85..96f6512d 100644 --- 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 index 0347a805..cd472c44 100644 --- a/packages/tests-e2e/tests/appRouter/ssr.test.ts +++ b/packages/tests-e2e/tests/appRouter/ssr.test.ts @@ -15,7 +15,7 @@ test("Server Side Render and loading.tsx", async ({ page }) => { let lastTime = ""; for (let i = 0; i < 5; i++) { - page.reload(); + void page.reload(); loading = page.getByText("Loading..."); await expect(loading).toBeVisible(); diff --git a/packages/tests-e2e/tests/pagesRouter/ssr.test.ts b/packages/tests-e2e/tests/pagesRouter/ssr.test.ts index c3258733..0928b7da 100644 --- a/packages/tests-e2e/tests/pagesRouter/ssr.test.ts +++ b/packages/tests-e2e/tests/pagesRouter/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/tsconfig.eslint.json b/tsconfig.eslint.json new file mode 100644 index 00000000..5759ab38 --- /dev/null +++ b/tsconfig.eslint.json @@ -0,0 +1,19 @@ +{ + "compilerOptions": { + "strict": true, + }, + "include": [ + "**/examples/**/*.ts", + "**/examples/**/*.tsx", + "**/examples/**/*.js", + "**/example/**/*.js", + "**/example/**/*.ts", + "**/docs/**/*.js", + ".eslintrc.cjs" + // etc + ], + "exclude": [ + "node_modules", + "docs/out" + ] +} \ No newline at end of file