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.

Sign up for GitHub

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

Jump to bottom

next-dev: change cluster usage to child process + stabilise inspect port (#45745 #45745

Merged
merged 4 commits into from
Feb 10, 2023
Merged

next-dev: change cluster usage to child process + stabilise inspect port (#45745 #45745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2549b5
change cluster to child process
feedthejim Feb 9, 2023
862730d
Merge branch 'canary' into feedthejim/next-447-1307-issues-caused-by-…
feedthejim Feb 9, 2023
74858dd
close server before exit
feedthejim Feb 9, 2023
73d6ef4
fix test
feedthejim Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 95 additions & 54 deletions packages/next/src/cli/next-dev.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env node
import arg from 'next/dist/compiled/arg/index.js'
import { startServer } from '../server/lib/start-server'
import { startServer, WORKER_SELF_EXIT_CODE } from '../server/lib/start-server'
import { getPort, printAndExit } from '../server/lib/utils'
import * as Log from '../build/output/log'
import { startedDevelopmentServer } from '../build/output'
Expand All @@ -13,20 +13,23 @@ import type { NextConfig } from '../../types'
import type { NextConfigComplete } from '../server/config-shared'
import { traceGlobals } from '../trace/shared'
import { isIPv6 } from 'net'
import cluster from 'cluster'
import { ChildProcess, fork } from 'child_process'
import { Telemetry } from '../telemetry/storage'
import loadConfig from '../server/config'
import { findPagesDir } from '../lib/find-pages-dir'
import { fileExists } from '../lib/file-exists'
import Watchpack from 'next/dist/compiled/watchpack'
import stripAnsi from 'next/dist/compiled/strip-ansi'
import { warn } from '../build/output/log'

let isTurboSession = false
let sessionStopHandled = false
let sessionStarted = Date.now()
let dir: string
let unwatchConfigFiles: () => void

const isChildProcess = !!process.env.__NEXT_DEV_CHILD_PROCESS

const handleSessionStop = async () => {
if (sessionStopHandled) return
sessionStopHandled = true
Expand Down Expand Up @@ -81,7 +84,7 @@ const handleSessionStop = async () => {
process.exit(0)
}

if (cluster.isMaster) {
if (!isChildProcess) {
process.on('SIGINT', handleSessionStop)
process.on('SIGTERM', handleSessionStop)
} else {
Expand Down Expand Up @@ -432,16 +435,75 @@ If you cannot make the changes above, but still want to try out\nNext.js v13 wit
// we're using a sub worker to avoid memory leaks. When memory usage exceeds 90%, we kill the worker and restart it.
// this is a temporary solution until we can fix the memory leaks.
// the logic for the worker killing itself is in `packages/next/server/lib/start-server.ts`
if (!process.env.__NEXT_DISABLE_MEMORY_WATCHER && cluster.isMaster) {
if (!process.env.__NEXT_DISABLE_MEMORY_WATCHER && !isChildProcess) {
let config: NextConfig
let childProcess: ChildProcess | null = null

const isDebugging = process.execArgv.some((localArg) =>
localArg.startsWith('--inspect')
Copy link
Contributor

Choose a reason for hiding this comment

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

cause #46948

)

const isDebuggingWithBrk = process.execArgv.some((localArg) =>
localArg.startsWith('--inspect-brk')
)

const debugPort = (() => {
const debugPortStr = process.execArgv
.find(
(localArg) =>
localArg.startsWith('--inspect') ||
localArg.startsWith('--inspect-brk')
)
?.split('=')[1]
return debugPortStr ? parseInt(debugPortStr, 10) : 9229
})()

if (isDebugging || isDebuggingWithBrk) {
warn(
Copy link
Member

Choose a reason for hiding this comment

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

Is this log necessary since Node.js already prints it's own log with the related port?

warn  - the --inspect option was detected, the Next.js server should be inspected at port 9230.
Debugger listening on ws://127.0.0.1:9230/fcbc278e-6902-436c-83f6-7057f1ad1aaf
For help, see: https://nodejs.org/en/docs/inspector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to say which one to use since it's confusing which one to connect to when there's two of them

`the --inspect${
isDebuggingWithBrk ? '-brk' : ''
} option was detected, the Next.js server should be inspected at port ${
debugPort + 1
}.`
)
}

const genExecArgv = () => {
const execArgv = process.execArgv.filter((localArg) => {
return (
!localArg.startsWith('--inspect') &&
!localArg.startsWith('--inspect-brk')
)
})

const setupFork = (env?: Parameters<typeof cluster.fork>[0]) => {
if (isDebugging || isDebuggingWithBrk) {
execArgv.push(
`--inspect${isDebuggingWithBrk ? '-brk' : ''}=${debugPort + 1}`
)
}

return execArgv
}

const setupFork = (env?: NodeJS.ProcessEnv, newDir?: string) => {
const startDir = dir
const [, script, ...nodeArgs] = process.argv
let shouldFilter = false
cluster.fork({
...env,
FORCE_COLOR: '1',
})
childProcess = fork(
newDir ? script.replace(startDir, newDir) : script,
nodeArgs,
{
env: {
...(env ? env : process.env),
FORCE_COLOR: '1',
__NEXT_DEV_CHILD_PROCESS: '1',
},
// @ts-ignore TODO: remove ignore when types are updated
windowsHide: true,
stdio: ['ipc', 'pipe', 'pipe'],
execArgv: genExecArgv(),
}
)

// since errors can start being logged from the fork
// before we detect the project directory rename
Expand Down Expand Up @@ -473,40 +535,27 @@ If you cannot make the changes above, but still want to try out\nNext.js v13 wit
process[fd].write(chunk)
}

for (const workerId in cluster.workers) {
cluster.workers[workerId]?.process.stdout?.on('data', (chunk) => {
filterForkErrors(chunk, 'stdout')
})
cluster.workers[workerId]?.process.stderr?.on('data', (chunk) => {
filterForkErrors(chunk, 'stderr')
})
}
}

const handleClusterExit = () => {
const callback = async (worker: cluster.Worker) => {
// ignore if we killed the worker
if ((worker as any).killed) return
childProcess?.stdout?.on('data', (chunk) => {
filterForkErrors(chunk, 'stdout')
})
childProcess?.stderr?.on('data', (chunk) => {
filterForkErrors(chunk, 'stderr')
})

// TODO: we should track how many restarts are
// occurring and how long in-between them
if (worker.exitedAfterDisconnect) {
const callback = async (code: number | null) => {
if (code === WORKER_SELF_EXIT_CODE) {
setupFork()
} else if (!sessionStopHandled) {
await handleSessionStop()
process.exit(1)
}
}
cluster.addListener('exit', callback)
return () => cluster.removeListener('exit', callback)
childProcess?.addListener('exit', callback)
return () => childProcess?.removeListener('exit', callback)
}
let clusterExitUnsub = handleClusterExit()
// x-ref: https://nodejs.org/api/cluster.html#clustersettings
// @ts-expect-error type is incorrect
cluster.settings.windowsHide = true
cluster.settings.stdio = ['ipc', 'pipe', 'pipe']

setupFork()
let childProcessExitUnsub = setupFork()

config = await loadConfig(
PHASE_DEVELOPMENT_SERVER,
dir,
Expand All @@ -516,27 +565,19 @@ If you cannot make the changes above, but still want to try out\nNext.js v13 wit
)

const handleProjectDirRename = (newDir: string) => {
clusterExitUnsub()

for (const workerId in cluster.workers) {
try {
// @ts-expect-error custom field
cluster.workers[workerId].killed = true
cluster.workers[workerId]!.process.kill('SIGKILL')
} catch (_) {}
}
childProcessExitUnsub()
childProcess?.kill()
process.chdir(newDir)
// @ts-expect-error type is incorrect
cluster.settings.cwd = newDir
cluster.settings.exec = cluster.settings.exec?.replace(dir, newDir)
setupFork({
...Object.keys(process.env).reduce((newEnv, key) => {
newEnv[key] = process.env[key]?.replace(dir, newDir)
return newEnv
}, {} as typeof process.env),
NEXT_PRIVATE_DEV_DIR: newDir,
})
clusterExitUnsub = handleClusterExit()
childProcessExitUnsub = setupFork(
{
...Object.keys(process.env).reduce((newEnv, key) => {
newEnv[key] = process.env[key]?.replace(dir, newDir)
return newEnv
}, {} as typeof process.env),
NEXT_PRIVATE_DEV_DIR: newDir,
},
newDir
)
}
const parentDir = path.join('/', dir, '..')
const watchedEntryLength = parentDir.split('/').length + 1
Expand Down
12 changes: 9 additions & 3 deletions packages/next/src/server/lib/start-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@ import { warn } from '../../build/output/log'
import http from 'http'
import next from '../next'
import { isIPv6 } from 'net'
import cluster from 'cluster'
import v8 from 'v8'
const isChildProcess = !!process.env.__NEXT_DEV_CHILD_PROCESS

interface StartServerOptions extends NextServerOptions {
allowRetry?: boolean
keepAliveTimeout?: number
}

export const WORKER_SELF_EXIT_CODE = 77

const MAXIMUM_HEAP_SIZE_ALLOWED =
(v8.getHeapStatistics().heap_size_limit / 1024 / 1024) * 0.9

Expand All @@ -20,10 +22,14 @@ export function startServer(opts: StartServerOptions) {
const server = http.createServer((req, res) => {
return requestHandler(req, res).finally(() => {
if (
cluster.worker &&
isChildProcess &&
process.memoryUsage().heapUsed / 1024 / 1024 > MAXIMUM_HEAP_SIZE_ALLOWED
) {
cluster.worker.kill()
warn(
'The server is running out of memory, restarting to free up memory.'
)
server.close()
process.exit(WORKER_SELF_EXIT_CODE)
}
})
})
Expand Down