-
-
Notifications
You must be signed in to change notification settings - Fork 814
fix: restore runtime close hook and implement graceful shutdown env vars
#4017
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
base: main
Are you sure you want to change the base?
Changes from 17 commits
cd7850a
4218d87
3482064
853f67f
ea4235a
63e2ebd
77f90e4
70627b2
6261cd6
8e1f996
5ebc5f9
ad08c19
b25ed2f
8769db6
2127dc3
4b68824
8fc90dd
12a6cac
9c5aee1
6330afe
0a7dc65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,16 @@ NITRO_PRESET=deno_server npm run build | |||||
| deno run --unstable --allow-net --allow-read --allow-env .output/server/index.ts | ||||||
| ``` | ||||||
|
|
||||||
| ### Environment Variables | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same heading-level skip as bun.md β should be The file uses π Suggested fix-### Environment Variables
+## Environment Variablesπ Committable suggestion
Suggested change
π§° Toolsπͺ markdownlint-cli2 (0.20.0)[warning] 21-21: Heading levels should only increment by one level at a time (MD001, heading-increment) π€ Prompt for AI Agents |
||||||
|
|
||||||
| You can customize server behavior using following environment variables: | ||||||
|
|
||||||
| - `NITRO_PORT` or `PORT` (defaults to `3000`) | ||||||
| - `NITRO_HOST` or `HOST` | ||||||
| - `NITRO_SSL_CERT` and `NITRO_SSL_KEY` - if both are present, this will launch the server in HTTPS mode. In the vast majority of cases, this should not be used other than for testing, and the Nitro server should be run behind a reverse proxy like nginx or Cloudflare which terminates SSL. | ||||||
| - `NITRO_SHUTDOWN_DISABLED` - Disables the graceful shutdown feature when set to `'true'`. Defaults to `'false'`. | ||||||
| - `NITRO_SHUTDOWN_TIMEOUT` - Sets the amount of time (in milliseconds) before a forced shutdown occurs. Defaults to `5000` milliseconds. | ||||||
|
|
||||||
| ## Deno Deploy | ||||||
|
|
||||||
| :read-more{to="/deploy/providers/deno-deploy"} | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import wsAdapter from "crossws/adapters/node"; | |
| import { useNitroApp } from "nitro/app"; | ||
| import { startScheduleRunner } from "#nitro/runtime/task"; | ||
| import { trapUnhandledErrors } from "#nitro/runtime/error/hooks"; | ||
| import { resolveGracefulShutdownConfig, setupShutdownHooks } from "#nitro/runtime/shutdown"; | ||
| import { resolveWebsocketHooks } from "#nitro/runtime/app"; | ||
|
|
||
| const _parsedPort = Number.parseInt(process.env.NITRO_PORT ?? process.env.PORT ?? ""); | ||
|
|
@@ -13,7 +14,8 @@ const port = Number.isNaN(_parsedPort) ? 3000 : _parsedPort; | |
| const host = process.env.NITRO_HOST || process.env.HOST; | ||
| const cert = process.env.NITRO_SSL_CERT; | ||
| const key = process.env.NITRO_SSL_KEY; | ||
| // const socketPath = process.env.NITRO_UNIX_SOCKET; // TODO | ||
| const socketPath = process.env.NITRO_UNIX_SOCKET; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could introduce this also in srvx directly via an env without |
||
| const gracefulShutdown = resolveGracefulShutdownConfig(); | ||
|
|
||
| const nitroApp = useNitroApp(); | ||
|
|
||
|
|
@@ -22,6 +24,8 @@ const server = serve({ | |
| hostname: host, | ||
| tls: cert && key ? { cert, key } : undefined, | ||
| fetch: nitroApp.fetch, | ||
| gracefulShutdown, | ||
| node: socketPath ? { path: socketPath } : undefined, | ||
| }); | ||
|
|
||
| if (import.meta._websocket) { | ||
|
|
@@ -38,6 +42,7 @@ if (import.meta._websocket) { | |
| } | ||
|
|
||
| trapUnhandledErrors(); | ||
| setupShutdownHooks(); | ||
|
|
||
| // Scheduled tasks | ||
| if (import.meta._tasks) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { useNitroApp } from "../app.ts"; | ||
| import type { ServerOptions } from "srvx"; | ||
|
|
||
| export function resolveGracefulShutdownConfig(): ServerOptions["gracefulShutdown"] { | ||
| if (process.env.NITRO_SHUTDOWN_DISABLED === "true") { | ||
| return false; | ||
| } | ||
|
wadefletch marked this conversation as resolved.
|
||
|
|
||
| const timeoutMs = Number.parseInt(process.env.NITRO_SHUTDOWN_TIMEOUT ?? "", 10); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is best that we don't introduce new |
||
|
|
||
| if (timeoutMs > 0) { | ||
| // srvx expects timeout in seconds | ||
| return { gracefulTimeout: timeoutMs / 1000 }; | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| async function _shutdownHandler() { | ||
| try { | ||
| await useNitroApp().hooks?.callHook("close"); | ||
| } catch (error) { | ||
| console.error("[nitro] Error running close hook:", error); | ||
| } | ||
| } | ||
|
|
||
| export function setupShutdownHooks() { | ||
| if (typeof process !== "undefined" && process.on) { | ||
| process.on("SIGTERM", _shutdownHandler); | ||
| process.on("SIGINT", _shutdownHandler); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double signal delivery can invoke the close hook twice. In production, it's common for orchestrators (Docker, Kubernetes, systemd) to send SIGTERM and then follow up with SIGINT if the process hasn't exited. Both signals would trigger Consider guarding with a flag: Proposed fix+let _shutting = false;
+
async function _shutdownHandler() {
+ if (_shutting) {
+ return;
+ }
+ _shutting = true;
try {
await useNitroApp().hooks?.callHook("close");
} catch (error) {
console.error("[nitro] Error running close hook:", error);
}
}π§° Toolsπͺ Biome (2.3.13)[error] 21-21: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. (lint/correctness/useHookAtTopLevel) π€ Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I declined to change this to follow the pattern from the pre-existing error handling hooks. This is likely a good safety feature, though double-closes are in most cases no-op and/or can be handled in the application, depending on how abstracted/magic the hooks system should beβpreferably less, imo, since hooks are by nature kind of a break through the abstraction wall.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
βοΈ Learnings added
|
||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| const callHook = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| vi.mock("../../src/runtime/internal/app.ts", () => ({ | ||
| useNitroApp: () => ({ | ||
| hooks: { callHook }, | ||
| }), | ||
| })); | ||
|
|
||
| import { | ||
| resolveGracefulShutdownConfig, | ||
| setupShutdownHooks, | ||
| } from "../../src/runtime/internal/shutdown.ts"; | ||
|
|
||
| describe("resolveGracefulShutdownConfig", () => { | ||
| const env = process.env; | ||
|
|
||
| afterEach(() => { | ||
| process.env = env; | ||
| }); | ||
|
|
||
| it("returns undefined by default", () => { | ||
| process.env = { ...env }; | ||
| delete process.env.NITRO_SHUTDOWN_DISABLED; | ||
| delete process.env.NITRO_SHUTDOWN_TIMEOUT; | ||
| expect(resolveGracefulShutdownConfig()).toBeUndefined(); | ||
| }); | ||
|
|
||
| it.each([ | ||
| { value: "true", expected: false }, | ||
| { value: "false", expected: undefined }, | ||
| { value: "", expected: undefined }, | ||
| { value: "1", expected: undefined }, | ||
| { value: "yes", expected: undefined }, | ||
| ])("NITRO_SHUTDOWN_DISABLED=$value returns $expected", ({ value, expected }) => { | ||
| process.env = { ...env, NITRO_SHUTDOWN_DISABLED: value }; | ||
| delete process.env.NITRO_SHUTDOWN_TIMEOUT; | ||
| expect(resolveGracefulShutdownConfig()).toBe(expected); | ||
| }); | ||
|
|
||
| it("returns gracefulTimeout in seconds from NITRO_SHUTDOWN_TIMEOUT ms", () => { | ||
| process.env = { ...env, NITRO_SHUTDOWN_TIMEOUT: "10000" }; | ||
| delete process.env.NITRO_SHUTDOWN_DISABLED; | ||
| expect(resolveGracefulShutdownConfig()).toEqual({ gracefulTimeout: 10 }); | ||
| }); | ||
|
|
||
| it("disabled takes priority over timeout", () => { | ||
| process.env = { | ||
| ...env, | ||
| NITRO_SHUTDOWN_DISABLED: "true", | ||
| NITRO_SHUTDOWN_TIMEOUT: "10000", | ||
| }; | ||
| expect(resolveGracefulShutdownConfig()).toBe(false); | ||
| }); | ||
|
|
||
| it("ignores non-numeric timeout", () => { | ||
| process.env = { ...env, NITRO_SHUTDOWN_TIMEOUT: "abc" }; | ||
| delete process.env.NITRO_SHUTDOWN_DISABLED; | ||
| expect(resolveGracefulShutdownConfig()).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("setupShutdownHooks", () => { | ||
| let savedSIGTERM: Function[]; | ||
| let savedSIGINT: Function[]; | ||
|
|
||
| beforeEach(() => { | ||
| savedSIGTERM = process.listeners("SIGTERM").slice(); | ||
| savedSIGINT = process.listeners("SIGINT").slice(); | ||
| callHook.mockClear(); | ||
| callHook.mockResolvedValue(undefined); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| process.removeAllListeners("SIGTERM"); | ||
| process.removeAllListeners("SIGINT"); | ||
| for (const fn of savedSIGTERM) process.on("SIGTERM", fn as () => void); | ||
| for (const fn of savedSIGINT) process.on("SIGINT", fn as () => void); | ||
| }); | ||
|
|
||
| it("registers SIGTERM and SIGINT handlers", () => { | ||
| const beforeTERM = process.listenerCount("SIGTERM"); | ||
| const beforeINT = process.listenerCount("SIGINT"); | ||
| setupShutdownHooks(); | ||
| expect(process.listenerCount("SIGTERM")).toBe(beforeTERM + 1); | ||
| expect(process.listenerCount("SIGINT")).toBe(beforeINT + 1); | ||
| }); | ||
|
|
||
| it("calls close hook on SIGTERM", async () => { | ||
| setupShutdownHooks(); | ||
| process.emit("SIGTERM", "SIGTERM"); | ||
| await vi.waitFor(() => { | ||
| expect(callHook).toHaveBeenCalledWith("close"); | ||
| }); | ||
| }); | ||
|
|
||
| it("calls close hook on SIGINT", async () => { | ||
| setupShutdownHooks(); | ||
| process.emit("SIGINT", "SIGINT"); | ||
| await vi.waitFor(() => { | ||
| expect(callHook).toHaveBeenCalledWith("close"); | ||
| }); | ||
| }); | ||
|
|
||
| it("awaits the close hook promise", async () => { | ||
| let resolved = false; | ||
| callHook.mockImplementation( | ||
| () => | ||
| new Promise<void>((r) => { | ||
| setTimeout(() => { | ||
| resolved = true; | ||
| r(); | ||
| }, 50); | ||
| }) | ||
| ); | ||
| setupShutdownHooks(); | ||
| process.emit("SIGTERM", "SIGTERM"); | ||
| await vi.waitFor(() => expect(resolved).toBe(true)); | ||
| }); | ||
|
|
||
| it("logs error if close hook throws", async () => { | ||
| const error = new Error("cleanup failed"); | ||
| callHook.mockRejectedValueOnce(error); | ||
| const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); | ||
| setupShutdownHooks(); | ||
| process.emit("SIGTERM", "SIGTERM"); | ||
| await vi.waitFor(() => { | ||
| expect(consoleSpy).toHaveBeenCalledWith("[nitro] Error running close hook:", error); | ||
| }); | ||
| consoleSpy.mockRestore(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heading level skips from h1 to h3.
There's no h2 between
# Bun(line 5) and### Environment Variables(line 21). This should be##to comply with Markdown heading hierarchy.π Suggested fix
π€ Prompt for AI Agents