From 61ca024536753f38bf625608eb79fb6f3e4377ef Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Wed, 31 May 2023 19:00:55 -0400 Subject: [PATCH] [turbopack]: Fix HEAD requests (#50366) I noticed while testing that we're getting a bunch of 500 errors after #50241 merged. Turns out that `fetchNextData` will [fetch `HEAD` requests](https://github.com/vercel/next.js/blob/cf9591cd/packages/next/src/shared/lib/router/router.ts#L619-L621) for background priority. The problem is that, somewhere, the Next router is draining the body from HEAD responses, leading us to trying to `JSON.parse` an empty string. This changes the way we return results to the Turbopack router. Instead of `JSON.stringify`ing the result into the body (which will be drained by something), we directly return the result. And it saves us a `stringify` -> `parse` -> `stringify` round trip, so that's nice. I took the chance to clean up some of our boilerplate code, too. --- .../crates/next-core/js/src/entry/router.ts | 91 ++++++++----------- .../next-core/js/src/internal/server.ts | 68 ++++---------- .../next/src/server/lib/route-resolver.ts | 29 ++++-- 3 files changed, 80 insertions(+), 108 deletions(-) diff --git a/packages/next-swc/crates/next-core/js/src/entry/router.ts b/packages/next-swc/crates/next-core/js/src/entry/router.ts index 9733a4f2e09d1..efd9575456680 100644 --- a/packages/next-swc/crates/next-core/js/src/entry/router.ts +++ b/packages/next-swc/crates/next-core/js/src/entry/router.ts @@ -1,12 +1,9 @@ import type { Ipc, StructuredError } from '@vercel/turbopack-node/ipc/index' -import type { IncomingMessage, ServerResponse } from 'node:http' +import type { IncomingMessage } from 'node:http' import { Buffer } from 'node:buffer' import { createServer, makeRequest, type ServerInfo } from '../internal/server' import { toPairs } from '../internal/headers' -import { - makeResolver, - type RouteResult, -} from 'next/dist/server/lib/route-resolver' +import { makeResolver } from 'next/dist/server/lib/route-resolver' import loadConfig from 'next/dist/server/config' import { PHASE_DEVELOPMENT_SERVER } from 'next/dist/shared/lib/constants' @@ -110,26 +107,46 @@ export default async function route( // the serverRequest to Next.js to handle. clientRequest.end(body) - // The route promise must not block us from starting the client response - // handling, so we cannot await it yet. By making the call, we allow - // Next.js to start writing to the response whenever it's ready. + // The route promise must not block us from starting the middleware + // response handling, so we cannot await it yet. By making the call, we + // allow Next.js to start writing to the response whenever it's ready. const routePromise = resolveRoute(serverRequest, serverResponse) - // Now that the Next.js has started processing the route, the - // clientResponsePromise will resolve once they write data and then we can - // begin streaming. - // We again cannot block on the clientResponsePromise, because an error may + // Now that the Next.js has started processing the route, the middleware + // response promise will resolve once they write data and then we can begin + // streaming. + // We again cannot await directly on the promise, because an error may // occur in the routePromise while we're waiting. - const responsePromise = clientResponsePromise.then((c) => - handleClientResponse(ipc, c) + const middlewarePromise = clientResponsePromise.then((c) => + handleMiddlewareResponse(ipc, c) ) // Now that both promises are in progress, we await both so that a // rejection in either will end the routing. - const [response] = await Promise.all([responsePromise, routePromise]) + const [routeResult] = await Promise.all([routePromise, middlewarePromise]) server.close() - return response + + if (routeResult) { + switch (routeResult.type) { + case 'none': + case 'error': + return routeResult + case 'rewrite': + return { + type: 'rewrite', + data: { + url: routeResult.url, + headers: Object.entries(routeResult.headers) + .filter(([, val]) => val != null) + .map(([name, value]) => [name, value!.toString()]), + }, + } + default: + // @ts-expect-error data.type is never + throw new Error(`unknown route result type: ${data.type}`) + } + } } catch (e) { // Server doesn't need to be closed, because the sendError will terminate // the process. @@ -137,44 +154,14 @@ export default async function route( } } -async function handleClientResponse( +async function handleMiddlewareResponse( ipc: Ipc, clientResponse: IncomingMessage -): Promise { - if (clientResponse.headers['x-nextjs-route-result'] === '1') { - clientResponse.setEncoding('utf8') - // We're either a redirect or a rewrite - let buffer = '' - for await (const chunk of clientResponse) { - buffer += chunk - } - - const data = JSON.parse(buffer) as RouteResult - - switch (data.type) { - case 'none': - return { - type: 'none', - } - case 'error': - return { - type: 'error', - error: data.error, - } - case 'rewrite': - return { - type: 'rewrite', - data: { - url: data.url, - headers: Object.entries(data.headers) - .filter(([, val]) => val != null) - .map(([name, value]) => [name, value!.toString()]), - }, - } - default: - // @ts-expect-error data.type is never - throw new Error(`unknown route result type: ${data.type}`) - } +): Promise { + // If this header is specified, we know that the response was not handled by + // middleware. The headers and body of the response are useless. + if (clientResponse.headers['x-nextjs-route-result']) { + return } const responseHeaders: MiddlewareHeadersResponse = { diff --git a/packages/next-swc/crates/next-core/js/src/internal/server.ts b/packages/next-swc/crates/next-core/js/src/internal/server.ts index 1157ad8ee6b47..4d9c87f8fd0b7 100644 --- a/packages/next-swc/crates/next-core/js/src/internal/server.ts +++ b/packages/next-swc/crates/next-core/js/src/internal/server.ts @@ -40,7 +40,6 @@ export function makeRequest( serverResponse: ServerResponse }> { return new Promise((resolve, reject) => { - let clientRequest: ClientRequest | null = null let clientResponseResolve: (value: IncomingMessage) => void let clientResponseReject: (error: Error) => void const clientResponsePromise = new Promise( @@ -49,32 +48,9 @@ export function makeRequest( clientResponseReject = reject } ) - let serverRequest: IncomingMessage | null = null - let serverResponse: ServerResponse | null = null - - const maybeResolve = () => { - if ( - clientRequest != null && - serverRequest != null && - serverResponse != null - ) { - cleanup() - resolve({ - clientRequest, - clientResponsePromise, - serverRequest, - serverResponse, - }) - } - } - - const cleanup = () => { - server.removeListener('error', errorListener) - server.removeListener('request', requestListener) - } const errorListener = (err: Error) => { - cleanup() + server.removeListener('request', requestListener) reject(err) } @@ -82,26 +58,13 @@ export function makeRequest( req: IncomingMessage, res: ServerResponse ) => { - serverRequest = req - serverResponse = res - maybeResolve() - } - - const cleanupClientResponse = () => { - if (clientRequest != null) { - clientRequest.removeListener('response', responseListener) - clientRequest.removeListener('error', clientResponseErrorListener) - } - } - - const clientResponseErrorListener = (err: Error) => { - cleanupClientResponse() - clientResponseReject(err) - } - - const responseListener = (res: IncomingMessage) => { - cleanupClientResponse() - clientResponseResolve(res) + server.removeListener('error', errorListener) + resolve({ + clientRequest, + clientResponsePromise, + serverRequest: req, + serverResponse: res, + }) } server.once('request', requestListener) @@ -112,18 +75,27 @@ export function makeRequest( const headers = headersFromEntries(rawHeaders ?? []) initProxiedHeaders(headers, proxiedFor) - clientRequest = http.request({ + const clientRequest = http.request({ host: 'localhost', port: address.port, method, - path: - rawQuery != null && rawQuery.length > 0 ? `${path}?${rawQuery}` : path, + path: rawQuery?.length ? `${path}?${rawQuery}` : path, headers, }) // Otherwise Node.js waits for the first chunk of data to be written before sending the request. clientRequest.flushHeaders() + const clientResponseErrorListener = (err: Error) => { + clientRequest.removeListener('response', responseListener) + clientResponseReject(err) + } + + const responseListener = (res: IncomingMessage) => { + clientRequest.removeListener('error', clientResponseErrorListener) + clientResponseResolve(res) + } + clientRequest.once('response', responseListener) clientRequest.once('error', clientResponseErrorListener) }) diff --git a/packages/next/src/server/lib/route-resolver.ts b/packages/next/src/server/lib/route-resolver.ts index d94c3944ea933..502ec8b2810b1 100644 --- a/packages/next/src/server/lib/route-resolver.ts +++ b/packages/next/src/server/lib/route-resolver.ts @@ -240,7 +240,7 @@ export async function makeResolver( return async function resolveRoute( _req: IncomingMessage, _res: ServerResponse - ) { + ): Promise { const req = new NodeNextRequest(_req) const res = new NodeNextResponse(_res) const parsedUrl = url.parse(req.url!, true) @@ -250,15 +250,28 @@ export async function makeResolver( await router.execute(req, res, parsedUrl) - if (!res.originalResponse.headersSent) { - res.setHeader('x-nextjs-route-result', '1') - const routeResult: RouteResult = routeResults.get(req) ?? { - type: 'none', - } + // If the headers are sent, then this was handled by middleware and there's + // nothing for us to do. + if (res.originalResponse.headersSent) { + return + } - res.body(JSON.stringify(routeResult)).send() + // The response won't be used, but we need to close the request so that the + // ClientResponse's promise will resolve. We signal that this response is + // unneeded via the header. + res.setHeader('x-nextjs-route-result', '1') + res.send() + + // If we have a routeResult, then we hit the catchAllRoute during execution + // and this is a rewrite request. + const routeResult = routeResults.get(req) + if (routeResult) { + routeResults.delete(req) + return routeResult } - routeResults.delete(req) + // Finally, if the catchall didn't match, than this request is invalid + // (maybe they're missing the basePath?) + return { type: 'none' } } }