Skip to content

Commit

Permalink
[turbopack]: Fix HEAD requests (vercel#50366)
Browse files Browse the repository at this point in the history 
I noticed while testing that we're getting a bunch of 500 errors after vercel#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.
  • Loading branch information
@jridgewell @hydRAnger
jridgewell authored and hydRAnger committed Jun 12, 2023
1 parent 19eef8d commit 61ca024
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 108 deletions.
91 changes: 39 additions & 52 deletions packages/next-swc/crates/next-core/js/src/entry/router.ts
View file
@@ -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'

Expand Down Expand Up @@ -110,71 +107,61 @@ 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.
ipc.sendError(e as Error)
}
}

async function handleClientResponse(
async function handleMiddlewareResponse(
ipc: Ipc<RouterRequest, IpcOutgoingMessage>,
clientResponse: IncomingMessage
): Promise<MessageData | void> {
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<void> {
// 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 = {
Expand Down
68 changes: 20 additions & 48 deletions packages/next-swc/crates/next-core/js/src/internal/server.ts
View file
Expand Up @@ -40,7 +40,6 @@ export function makeRequest(
serverResponse: ServerResponse<IncomingMessage>
}> {
return new Promise((resolve, reject) => {
let clientRequest: ClientRequest | null = null
let clientResponseResolve: (value: IncomingMessage) => void
let clientResponseReject: (error: Error) => void
const clientResponsePromise = new Promise<IncomingMessage>(
Expand All @@ -49,59 +48,23 @@ export function makeRequest(
clientResponseReject = reject
}
)
let serverRequest: IncomingMessage | null = null
let serverResponse: ServerResponse<IncomingMessage> | 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)
}

const requestListener = (
req: IncomingMessage,
res: ServerResponse<IncomingMessage>
) => {
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)
Expand All @@ -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)
})
Expand Down
29 changes: 21 additions & 8 deletions packages/next/src/server/lib/route-resolver.ts
View file
Expand Up @@ -240,7 +240,7 @@ export async function makeResolver(
return async function resolveRoute(
_req: IncomingMessage,
_res: ServerResponse
) {
): Promise<RouteResult | void> {
const req = new NodeNextRequest(_req)
const res = new NodeNextResponse(_res)
const parsedUrl = url.parse(req.url!, true)
Expand All @@ -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' }
}
}

0 comments on commit 61ca024

Please sign in to comment.