From 14bee293197d1759ffbb474d4c292acb4fd9a5c8 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 17 Oct 2023 14:04:15 -0600 Subject: [PATCH] fix: fix hanging on broken responses --- packages/next/src/server/base-http/web.ts | 7 +-- packages/next/src/server/pipe-readable.ts | 45 +++++++++++-------- .../spec-extension/adapters/next-request.ts | 30 ++++++------- 3 files changed, 42 insertions(+), 40 deletions(-) diff --git a/packages/next/src/server/base-http/web.ts b/packages/next/src/server/base-http/web.ts index 88b55c58668dc..a77ec0460b3df 100644 --- a/packages/next/src/server/base-http/web.ts +++ b/packages/next/src/server/base-http/web.ts @@ -1,12 +1,9 @@ import type { IncomingHttpHeaders, OutgoingHttpHeaders } from 'http' import type { FetchMetrics } from './index' -// This takes advantage of `Promise.withResolvers` which is polyfilled in -// this imported module. -import '../../lib/polyfill-promise-with-resolvers' - import { toNodeOutgoingHttpHeaders } from '../web/utils' import { BaseNextRequest, BaseNextResponse } from './index' +import { DetachedPromise } from '../../lib/detached-promise' export class WebNextRequest extends BaseNextRequest { public request: Request @@ -87,7 +84,7 @@ export class WebNextResponse extends BaseNextResponse { return this } - private readonly sendPromise = Promise.withResolvers() + private readonly sendPromise = new DetachedPromise() private _sent = false public send() { this.sendPromise.resolve() diff --git a/packages/next/src/server/pipe-readable.ts b/packages/next/src/server/pipe-readable.ts index 1ec4bb3e11630..1d9602b103e92 100644 --- a/packages/next/src/server/pipe-readable.ts +++ b/packages/next/src/server/pipe-readable.ts @@ -1,35 +1,37 @@ import type { ServerResponse } from 'node:http' import './node-polyfill-web-streams' -import { abortControllerFromNodeResponse } from './web/spec-extension/adapters/next-request' + +import { createAbortController } from './web/spec-extension/adapters/next-request' +import { DetachedPromise } from '../lib/detached-promise' export function isAbortError(e: any): e is Error & { name: 'AbortError' } { return e?.name === 'AbortError' } function createWriterFromResponse( - res: ServerResponse, - controller: AbortController + res: ServerResponse ): WritableStream { let started = false // Create a promise that will resolve once the response has drained. See // https://nodejs.org/api/stream.html#stream_event_drain - let drained = Promise.withResolvers() - res.on('drain', () => { + let drained = new DetachedPromise() + function onDrain() { drained.resolve() - }) + } + res.on('drain', onDrain) - // Create a promise that will resolve once the response has finished. See - // https://nodejs.org/api/http.html#event-finish_1 - const finished = Promise.withResolvers() + // If the finish event fires, it means we shouldn't block and wait for the + // drain event. res.once('close', () => { - // If the finish event fires, it means we shouldn't block and wait for the - // drain event. + res.off('drain', onDrain) drained.resolve() }) - // Once the response finishes, resolve the promise. + // Create a promise that will resolve once the response has finished. See + // https://nodejs.org/api/http.html#event-finish_1 + const finished = new DetachedPromise() res.once('finish', () => { finished.resolve() }) @@ -57,16 +59,21 @@ function createWriterFromResponse( // If the write returns false, it means there's some backpressure, so // wait until it's streamed before continuing. if (!ok) { - // Reset the drained promise so that we can wait for the next drain event. - drained = Promise.withResolvers() - await drained.promise + + // Reset the drained promise so that we can wait for the next drain event. + drained = new DetachedPromise() } - } catch (err: any) { - controller.abort(err) + } catch (err) { + res.end() throw err } }, + abort: (err) => { + if (res.writableFinished) return + + res.destroy(err) + }, close: () => { if (res.writableFinished) return @@ -87,9 +94,9 @@ export async function pipeToNodeResponse( // Create a new AbortController so that we can abort the readable if the // client disconnects. - const controller = abortControllerFromNodeResponse(res) + const controller = createAbortController(res) - const writer = createWriterFromResponse(res, controller) + const writer = createWriterFromResponse(res) await readable.pipeTo(writer, { signal: controller.signal }) } catch (err: any) { diff --git a/packages/next/src/server/web/spec-extension/adapters/next-request.ts b/packages/next/src/server/web/spec-extension/adapters/next-request.ts index e0e603d06a710..72cee96609479 100644 --- a/packages/next/src/server/web/spec-extension/adapters/next-request.ts +++ b/packages/next/src/server/web/spec-extension/adapters/next-request.ts @@ -7,25 +7,24 @@ import { getRequestMeta } from '../../../request-meta' import { fromNodeOutgoingHttpHeaders } from '../../utils' import { NextRequest } from '../request' -export function abortControllerFromNodeResponse( - response: Writable -): AbortController { +/** + * Creates an AbortController tied to the closing of a ServerResponse (or other + * appropriate Writable). + * + * If the `close` event is fired before the `finish` event, then we'll send the + * `abort` signal. + */ +export function createAbortController(response: Writable): AbortController { const controller = new AbortController() // If `finish` fires first, then `res.end()` has been called and the close is // just us finishing the stream on our side. If `close` fires first, then we // know the client disconnected before we finished. - function onClose() { - controller.abort() - // eslint-disable-next-line @typescript-eslint/no-use-before-define - response.off('finish', onFinish) - } - function onFinish() { - response.off('close', onClose) - } + response.once('close', () => { + if (response.writableFinished) return - response.once('close', onClose) - response.once('finish', onFinish) + controller.abort() + }) return controller } @@ -42,9 +41,8 @@ export function signalFromNodeResponse(response: Writable): AbortSignal { const { errored, destroyed } = response if (errored || destroyed) return AbortSignal.abort(errored) - const controller = abortControllerFromNodeResponse(response) - - return controller.signal + const { signal } = createAbortController(response) + return signal } export class NextRequestAdapter {