Skip to content

Commit

Permalink
remove inheritance from NextCustomServer (vercel#73005)
Browse files Browse the repository at this point in the history
### What

This PR changes `NextCustomServer` to no longer subclass `NextServer` and instead just delegate all methods to `this.renderServer`. I've also added an interface implemented by both `NextServer` and `NextCustomServer` to make it explicit what we're actually exposing. (Note that we're currently exposing too much for legacy reasons. we might want to decide to clean this up a bit, i've started that in vercel#73021)

I've also cleaned up some random `(_ as any)`s and unnecessary indirections around the area.

### Why

currently, `NextCustomServer` is a subclass of `NextServer`, which makes it very confusing to reason about, because in  `prepare()` it creates *a nested `NextServer`* (stored in `this.renderServer`). this isn't immediately visible in the code, but that's what `getRequestHandlers` ends up doing.
https://github.com/vercel/next.js/blob/58b993c239689f22641682bbcb0d8dea6d85fdf1/packages/next/src/server/next.ts#L281
then, it delegates *some* methods to that (`render`, `getRequestHandler`, `setAssetPrefix`). But some methods (e.g. `render404`) *aren't* overridden/delegated and just run the implementation inherited from `NextServer`. And those implementations are unaware of `this.renderServer`, so they end up initializing a *second* `NextNodeServer` (`this.server` vs `this.renderServer.server`) and run against that instead. that's bad! and it's why e.g. vercel#61676 was necessary -- we needed to update both servers! 

i think that this is a leftover of the old implementation where we ran a separate process for rendering. for historical context, see:
- vercel#49805 which seems to have introduced this split (the proxy only overrides `prepare`, `getRequestHandler`, and `render`)
- vercel#53523 which turned the proxy code into `NextCustomServer`

also apparently `render404` and others were broken in development (try running the tests in the first commit
I've changed `NextCustomServer` and see the server hang forever).
  • Loading branch information
lubieowoce authored Nov 22, 2024
1 parent 87c74c8 commit 08e7410
Show file tree
Hide file tree
Showing 10 changed files with 351 additions and 108 deletions.
71 changes: 30 additions & 41 deletions packages/next/src/server/lib/render-server.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,17 @@
import type { NextServer, RequestHandler } from '../next'
import type { NextServer, RequestHandler, UpgradeHandler } from '../next'
import type { DevBundlerService } from './dev-bundler-service'
import type { PropagateToWorkersField } from './router-utils/types'

import next from '../next'
import type { Span } from '../../trace'

let initializations: Record<
string,
| Promise<{
requestHandler: ReturnType<
InstanceType<typeof NextServer>['getRequestHandler']
>
upgradeHandler: ReturnType<
InstanceType<typeof NextServer>['getUpgradeHandler']
>
app: ReturnType<typeof next>
}>
| undefined
> = {}
export type ServerInitResult = {
requestHandler: RequestHandler
upgradeHandler: UpgradeHandler
server: NextServer
}

let initializations: Record<string, Promise<ServerInitResult> | undefined> = {}

let sandboxContext: undefined | typeof import('../web/sandbox/context')

Expand All @@ -41,9 +35,9 @@ export async function getServerField(
if (!initialization) {
throw new Error('Invariant cant propagate server field, no app initialized')
}
const { app } = initialization
let appField = (app as any).server
return appField[field]
const { server } = initialization
let wrappedServer = server['server']! // NextServer.server is private
return wrappedServer[field as keyof typeof wrappedServer]
}

export async function propagateServerField(
Expand All @@ -55,17 +49,20 @@ export async function propagateServerField(
if (!initialization) {
throw new Error('Invariant cant propagate server field, no app initialized')
}
const { app } = initialization
let appField = (app as any).server
const { server } = initialization
let wrappedServer = server['server']
const _field = field as keyof NonNullable<typeof wrappedServer>

if (appField) {
if (typeof appField[field] === 'function') {
await appField[field].apply(
(app as any).server,
if (wrappedServer) {
if (typeof wrappedServer[_field] === 'function') {
// @ts-expect-error
await wrappedServer[_field].apply(
wrappedServer,
Array.isArray(value) ? value : []
)
} else {
appField[field] = value
// @ts-expect-error
wrappedServer[_field] = value
}
}
}
Expand All @@ -86,45 +83,37 @@ async function initializeImpl(opts: {
bundlerService: DevBundlerService | undefined
startServerSpan: Span | undefined
quiet?: boolean
}) {
}): Promise<ServerInitResult> {
const type = process.env.__NEXT_PRIVATE_RENDER_WORKER
if (type) {
process.title = 'next-render-worker-' + type
}

let requestHandler: RequestHandler
let upgradeHandler: any
let upgradeHandler: UpgradeHandler

const app = next({
const server = next({
...opts,
hostname: opts.hostname || 'localhost',
customServer: false,
httpServer: opts.server,
port: opts.port,
})
requestHandler = app.getRequestHandler()
upgradeHandler = app.getUpgradeHandler()
}) as NextServer // should return a NextServer when `customServer: false`
requestHandler = server.getRequestHandler()
upgradeHandler = server.getUpgradeHandler()

await app.prepare(opts.serverFields)
await server.prepare(opts.serverFields)

return {
requestHandler,
upgradeHandler,
app,
server,
}
}

export async function initialize(
opts: Parameters<typeof initializeImpl>[0]
): Promise<{
requestHandler: ReturnType<
InstanceType<typeof NextServer>['getRequestHandler']
>
upgradeHandler: ReturnType<
InstanceType<typeof NextServer>['getUpgradeHandler']
>
app: NextServer
}> {
): Promise<ServerInitResult> {
// if we already setup the server return as we only need to do
// this on first worker boot
if (initializations[opts.dir]) {
Expand Down
6 changes: 3 additions & 3 deletions packages/next/src/server/lib/router-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import type { WorkerRequestHandler, WorkerUpgradeHandler } from './types'
import type { DevBundler, ServerFields } from './router-utils/setup-dev-bundler'
import type { NextUrlWithParsedQuery, RequestMeta } from '../request-meta'
import type { NextServer } from '../next'

// This is required before other imports to ensure the require hook is setup.
import '../node-environment'
Expand Down Expand Up @@ -47,6 +46,7 @@ import {
} from '../dev/hot-reloader-types'
import { normalizedAssetPrefix } from '../../shared/lib/normalized-asset-prefix'
import { NEXT_PATCH_SYMBOL } from './patch-fetch'
import type { ServerInitResult } from './render-server'

const debug = setupDebug('next:router-server:main')
const isNextFont = (pathname: string | null) =>
Expand Down Expand Up @@ -79,7 +79,7 @@ export async function initialize(opts: {
experimentalHttpsServer?: boolean
startServerSpan?: Span
quiet?: boolean
}): Promise<[WorkerRequestHandler, WorkerUpgradeHandler, NextServer]> {
}): Promise<ServerInitResult> {
if (!process.env.NODE_ENV) {
// @ts-ignore not readonly
process.env.NODE_ENV = opts.dev ? 'development' : 'production'
Expand Down Expand Up @@ -739,5 +739,5 @@ export async function initialize(opts: {
}
}

return [requestHandler, upgradeHandler, handlers.app]
return { requestHandler, upgradeHandler, server: handlers.server }
}
4 changes: 2 additions & 2 deletions packages/next/src/server/lib/start-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,8 @@ export async function startServer(
keepAliveTimeout,
experimentalHttpsServer: !!selfSignedCertificate,
})
requestHandler = initResult[0]
upgradeHandler = initResult[1]
requestHandler = initResult.requestHandler
upgradeHandler = initResult.upgradeHandler

const startServerProcessDuration =
performance.mark('next-start-end') &&
Expand Down
Loading

0 comments on commit 08e7410

Please sign in to comment.