Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[turbopack]: Fix HEAD requests #50366

Merged
merged 4 commits into from
May 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 39 additions & 52 deletions packages/next-swc/crates/next-core/js/src/entry/router.ts
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was erroring out for HEAD requests, because the clientResponse always has an empty body.


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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this call routeResults.delete(req)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it gets to this point, then middleware would have fully handled the response before the catchAllRoute handler, so there shouldn't be an entry in routeResults

}

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' }
}
}