diff --git a/.changeset/unhandled-errors-reach-host.md b/.changeset/unhandled-errors-reach-host.md new file mode 100644 index 000000000..9a6737943 --- /dev/null +++ b/.changeset/unhandled-errors-reach-host.md @@ -0,0 +1,5 @@ +--- +"@solidjs/start": patch +--- + +Let the server hosting the app answer and report an error that an API route or middleware throws, or that a server function request hits before the function runs. The handler answered it inside its own h3 app, so its `fetch` never rejected: with Nitro, the `error` hook and the `errorHandler` never ran for it, and SolidStart's own dev server never showed its error overlay. An h3 `HTTPError` not marked unhandled, or a status thrown as a number, is answered as before. A server that calls the handler's `fetch` itself now gets a rejected promise for such an error and answers it; headers set for the error response with `event.res.errHeaders` are not part of that answer. diff --git a/apps/tests/src/e2e/api-call.test.ts b/apps/tests/src/e2e/api-call.test.ts index 4665d75d5..1a87b36df 100644 --- a/apps/tests/src/e2e/api-call.test.ts +++ b/apps/tests/src/e2e/api-call.test.ts @@ -32,6 +32,14 @@ test.describe("api calls", () => { expect(head.headers.get("content-type")).toBe(get.headers.get("content-type")); }); + test("should pass an error thrown by an API route to Nitro's error hook", async () => { + const response = await fetch("http://localhost:3000/api/throws"); + + expect(response.status).toBe(500); + // Set by the Nitro plugin in `src/nitro-error-hook.ts` from Nitro's `error` hook. + expect(response.headers.get("x-nitro-error-hook")).toBe("thrown by an API route"); + }); + test("should preserve multiple Set-Cookie headers on redirect (RFC 6265)", async () => { const response = await fetch("http://localhost:3000/api/multi-set-cookie-redirect", { redirect: "manual", diff --git a/apps/tests/src/nitro-error-hook.ts b/apps/tests/src/nitro-error-hook.ts new file mode 100644 index 000000000..53c7bac5a --- /dev/null +++ b/apps/tests/src/nitro-error-hook.ts @@ -0,0 +1,18 @@ +import { definePlugin } from "nitro"; + +/** + * Names the error that Nitro's `error` hook received for a request in a header + * of its response, so a test can tell that the error reached the server. + */ +export default definePlugin(nitroApp => { + const errors = new WeakMap(); + + nitroApp.hooks.hook("error", (error, { event }) => { + if (event) errors.set(event.req, error.message); + }); + + nitroApp.hooks.hook("response", (response, event) => { + const message = errors.get(event.req); + if (message) response.headers.set("x-nitro-error-hook", message); + }); +}); diff --git a/apps/tests/src/routes/api/throws.ts b/apps/tests/src/routes/api/throws.ts new file mode 100644 index 000000000..1df44efd0 --- /dev/null +++ b/apps/tests/src/routes/api/throws.ts @@ -0,0 +1,3 @@ +export const GET = async () => { + throw new Error("thrown by an API route"); +}; diff --git a/apps/tests/vite.config.ts b/apps/tests/vite.config.ts index e7baed41b..9ba506bea 100644 --- a/apps/tests/vite.config.ts +++ b/apps/tests/vite.config.ts @@ -33,6 +33,7 @@ export default defineConfig({ }), nitro({ compressPublicAssets: true, + plugins: ["./src/nitro-error-hook.ts"], }), ], }); diff --git a/packages/start/src/config/dev-server.spec.ts b/packages/start/src/config/dev-server.spec.ts index d0e0b08fd..18cd29a9d 100644 --- a/packages/start/src/config/dev-server.spec.ts +++ b/packages/start/src/config/dev-server.spec.ts @@ -1,19 +1,28 @@ import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { createServer as createHttpServer } from "node:http"; +import type { AddressInfo } from "node:net"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { + type Connect, createServer, type DevEnvironment, type EnvironmentModuleNode, type HotUpdateOptions, normalizePath, type Plugin, + type PreviewServer, type ViteDevServer, } from "vite"; import { afterEach, describe, expect, it, vi } from "vitest"; import { VITE_ENVIRONMENTS } from "./constants.ts"; -import { isHtmlResponse, resolvePreviewServerEntry, serverOnlyReload } from "./dev-server.ts"; +import { + devServer, + isHtmlResponse, + resolvePreviewServerEntry, + serverOnlyReload, +} from "./dev-server.ts"; const temporaryDirectories: string[] = []; @@ -55,6 +64,58 @@ describe("resolvePreviewServerEntry", () => { }); }); +describe("the preview server", () => { + /** + * Serves a built server entry whose `fetch` rejects through the preview + * middleware, and answers the status of one request to it. + */ + async function preview(error: Error) { + const httpServer = createHttpServer(); + (globalThis as any).PREVIEW_ERROR = error; + try { + const { root, serverEntry } = createServerEntry("mjs"); + writeFileSync( + serverEntry, + `export default { fetch: () => Promise.reject(globalThis.PREVIEW_ERROR) };`, + ); + + let middleware: Connect.NextHandleFunction | undefined; + const previewServer = { + config: { root, plugins: [] }, + middlewares: { use: (fn: Connect.NextHandleFunction) => (middleware = fn) }, + } as unknown as PreviewServer; + const plugin = devServer("./src/entry-server.tsx")[1] as Plugin; + const configure = plugin.configurePreviewServer as (server: PreviewServer) => () => void; + configure(previewServer)(); + + httpServer.on("request", (req, res) => middleware!(req, res, () => {})); + await new Promise(resolve => httpServer.listen(0, "127.0.0.1", resolve)); + const { port } = httpServer.address() as AddressInfo; + const response = await fetch(`http://127.0.0.1:${port}/api`, { + signal: AbortSignal.timeout(2000), + }); + await response.body?.cancel(); + return response.status; + } finally { + delete (globalThis as any).PREVIEW_ERROR; + if (httpServer.listening) { + httpServer.closeAllConnections(); + await new Promise(resolve => httpServer.close(resolve)); + } + } + } + + it("answers with a 500 when the app leaves an error to it", async () => { + const error = new Error("thrown by an API route"); + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); + + const status = await preview(error); + + expect(status).toBe(500); + expect(consoleError).toHaveBeenCalledWith(error); + }); +}); + describe("serverOnlyReload", () => { /** An app whose middleware is configured without a file extension */ async function createApp() { diff --git a/packages/start/src/config/dev-server.ts b/packages/start/src/config/dev-server.ts index 0ed664ef2..293ab8c6d 100644 --- a/packages/start/src/config/dev-server.ts +++ b/packages/start/src/config/dev-server.ts @@ -29,14 +29,20 @@ export function devServer(serverEntryPath: string, middlewarePath?: string): Arr return () => { server.middlewares.use(async (req, res) => { const webReq = new NodeRequest({ req, res }); - const def: { - default: { fetch: (req: Request) => Promise }; - } = await import(serverEntryUrl); - const webRes = await def.default.fetch(webReq); - if (isHtmlResponse(webRes)) { - res.setHeader("content-encoding", "identity"); + try { + const def: { + default: { fetch: (req: Request) => Promise }; + } = await import(serverEntryUrl); + const webRes = await def.default.fetch(webReq); + if (isHtmlResponse(webRes)) { + res.setHeader("content-encoding", "identity"); + } + sendNodeResponse(res, webRes); + } catch (error) { + // The handler leaves an unhandled error to the server hosting it. + console.error(error); + sendNodeResponse(res, new Response("Internal Server Error", { status: 500 })); } - sendNodeResponse(res, webRes); }); }; }, diff --git a/packages/start/src/server/handler.spec.ts b/packages/start/src/server/handler.spec.ts new file mode 100644 index 000000000..cff586810 --- /dev/null +++ b/packages/start/src/server/handler.spec.ts @@ -0,0 +1,300 @@ +import { H3, HTTPError, type Middleware } from "h3"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import type { FetchEvent, StartHandler } from "./types.ts"; + +type APIHandler = (event: FetchEvent) => unknown; + +const app = vi.hoisted(() => ({ + middleware: [] as Middleware[], + GET: undefined as APIHandler | undefined, +})); + +vi.mock("solid-start:middleware", () => ({ + get default() { + return app.middleware; + }, +})); + +vi.mock("./api-routes.ts", () => ({ + matchAPIRoute: (path: string) => + path === "/api" && app.GET + ? { handler: { import: async () => ({ GET: app.GET }) }, isPage: false } + : undefined, +})); + +vi.mock("../router.tsx", () => ({ createRoutes: vi.fn() })); +vi.mock("../fns/handler.ts", () => ({ handleServerFunction: vi.fn() })); +vi.mock("./manifest/ssr-manifest.ts", () => ({ getSsrManifest: vi.fn() })); + +const createHandler = async () => { + const { createBaseHandler } = await import("./handler.ts"); + return createBaseHandler(vi.fn(), vi.fn()); +}; + +const request = (search = "") => new Request(`http://localhost/api${search}`); + +const throwing = (thrown: unknown) => () => { + throw thrown; +}; + +beforeEach(() => { + vi.resetModules(); + app.middleware = []; + app.GET = undefined; +}); + +describe("createBaseHandler", () => { + it("answers with what an API route returns", async () => { + app.GET = () => new Response("ok"); + const handler = await createHandler(); + + const response = await handler.fetch(request()); + + expect(response.status).toBe(200); + expect(await response.text()).toBe("ok"); + }); + + it("answers an HTTPError that an API route throws", async () => { + app.GET = throwing(new HTTPError({ status: 418, message: "teapot" })); + const handler = await createHandler(); + + const response = await handler.fetch(request()); + + expect(response.status).toBe(418); + expect(await response.json()).toMatchObject({ message: "teapot" }); + }); + + it("answers an HTTPError that middleware throws", async () => { + app.middleware = [throwing(new HTTPError({ status: 401, message: "sign in" }))]; + const handler = await createHandler(); + + const response = await handler.fetch(request()); + + expect(response.status).toBe(401); + expect(await response.json()).toMatchObject({ message: "sign in" }); + }); + + it("answers a status thrown as a number", async () => { + app.GET = throwing(404); + const handler = await createHandler(); + + const response = await handler.fetch(request()); + + expect(response.status).toBe(404); + }); + + it.each([ + ["not-found", "h3.notFound", 404], + ["handled", "h3.handled", 200], + ])("answers h3's %s marker as h3 does", async (_, key, status) => { + app.GET = throwing(Symbol.for(key)); + const handler = await createHandler(); + + const response = await handler.fetch(request()); + + expect(response.status).toBe(status); + }); + + it("answers with what middleware returns for an error it catches", async () => { + app.GET = throwing(new Error("thrown by an API route")); + app.middleware = [ + async (_event, next) => { + try { + return await next(); + } catch { + return new Response("caught", { status: 503 }); + } + }, + ]; + const handler = await createHandler(); + + const response = await handler.fetch(request()); + + expect(response.status).toBe(503); + expect(await response.text()).toBe("caught"); + }); + + describe("unhandled errors", () => { + it("leaves an error that an API route throws to the server hosting the handler", async () => { + const error = new Error("thrown by an API route"); + app.GET = throwing(error); + const handler = await createHandler(); + + await expect(handler.fetch(request())).rejects.toBe(error); + }); + + it("leaves an error that middleware throws to the server hosting the handler", async () => { + const error = new Error("thrown by middleware"); + app.middleware = [throwing(error)]; + const handler = await createHandler(); + + await expect(handler.fetch(request())).rejects.toBe(error); + }); + + it.each([ + ["a string", "thrown by an API route"], + ["undefined", undefined], + ])("leaves %s, as h3 would answer it, to the server hosting the handler", async (_, thrown) => { + app.GET = throwing(thrown); + const handler = await createHandler(); + + const error = await handler.fetch(request()).then( + () => expect.unreachable(), + (error: unknown) => error, + ); + + expect(HTTPError.isError(error)).toBe(true); + expect(error).toMatchObject({ status: 500, unhandled: true, cause: thrown }); + }); + + it("answers it through request(), as h3 does", async () => { + app.GET = throwing(new Error("thrown by an API route")); + const handler = await createHandler(); + + const response = await handler.request("/api"); + + expect(response.status).toBe(500); + expect(await response.json()).toMatchObject({ status: 500, unhandled: true }); + }); + + it("leaves it to an h3 app the handler is mounted in", async () => { + const error = new Error("thrown by an API route"); + app.GET = throwing(error); + const onError = vi.fn(); + const outer = new H3({ onError }); + outer.mount("", await createHandler()); + + const response = await outer.fetch(request()); + + expect(response.status).toBe(500); + expect(onError).toHaveBeenCalledOnce(); + expect(onError.mock.calls[0]![0].cause).toBe(error); + }); + + it("leaves an HTTPError marked as unhandled to the server hosting the handler", async () => { + const error = new HTTPError({ status: 500, unhandled: true }); + app.GET = throwing(error); + const handler = await createHandler(); + + await expect(handler.fetch(request())).rejects.toBe(error); + }); + + it("leaves another library's HTTPError, as an unhandled 500, to the server hosting the handler", async () => { + // As ky and got name theirs + class LibraryHTTPError extends Error { + override name = "HTTPError"; + } + const thrown = new LibraryHTTPError("Request failed with status code 502"); + app.GET = throwing(thrown); + const handler = await createHandler(); + + const error = await handler.fetch(request()).then( + () => expect.unreachable(), + (error: unknown) => error, + ); + + expect(error).not.toBe(thrown); + expect(error).toMatchObject({ + status: 500, + unhandled: true, + cause: thrown, + message: "Request failed with status code 502", + }); + expect((error as Error).stack).toBe(thrown.stack); + }); + + it.each([ + ["different requests", () => [request("?1"), request("?2")]], + ["one request", () => Array(2).fill(request())], + ])("rejects only the call whose handler threw, for two calls with %s", async (_, requests) => { + const error = new Error("thrown by an API route"); + let release!: () => void; + const released = new Promise(resolve => (release = resolve)); + let calls = 0; + app.GET = async () => { + // The first call throws once the second one has been answered. + if (++calls > 1) return new Response("ok"); + await released; + throw error; + }; + const handler = await createHandler(); + + const settled = requests().map((req: Request) => handler.fetch(req)); + await Promise.race(settled); + release(); + const outcomes = await Promise.allSettled(settled); + + expect(outcomes.filter(outcome => outcome.status === "rejected")).toEqual([ + { status: "rejected", reason: error }, + ]); + const [answered] = outcomes.filter(outcome => outcome.status === "fulfilled"); + expect(await answered!.value.text()).toBe("ok"); + }); + + it("answers an error of a request() made during a call, as h3 does", async () => { + app.GET = throwing(new Error("thrown by an API route")); + let handler!: StartHandler; + let depth = 0; + app.middleware = [ + async (event, next) => { + if (depth++ > 0) return next(); + // The same request, handled again while the call runs + return handler.request(event.req); + }, + ]; + handler = await createHandler(); + + const response = await handler.fetch(request()); + + expect(response.status).toBe(500); + expect(await response.json()).toMatchObject({ status: 500, unhandled: true }); + }); + + it("leaves its own error to the server after a hook made another request() first", async () => { + const error = new Error("thrown by an API route"); + app.GET = event => { + if (new URL(event.request.url).searchParams.has("nested")) return new Response("nested"); + throw error; + }; + const handler = await createHandler(); + let nested = 0; + handler.config.onRequest = async () => { + if (nested++ === 0) await handler.request("/api?nested"); + }; + + await expect(handler.fetch(request())).rejects.toBe(error); + }); + + it("lets middleware recover an error of another handler it enters through `handler`", async () => { + app.GET = throwing(new Error("thrown by an API route")); + const inner = await createHandler(); + app.middleware = [ + async (_event, next) => { + try { + return await next(); + } catch { + return new Response("recovered"); + } + }, + event => inner.handler(event), + ]; + const handler = await createHandler(); + + const response = await handler.fetch(request()); + + expect(await response.text()).toBe("recovered"); + }); + + it("answers the next request", async () => { + app.GET = throwing(new Error("thrown by an API route")); + const handler = await createHandler(); + await expect(handler.fetch(request())).rejects.toThrow(); + + app.GET = () => new Response("ok"); + const response = await handler.fetch(request()); + + expect(await response.text()).toBe("ok"); + }); + }); +}); diff --git a/packages/start/src/server/handler.ts b/packages/start/src/server/handler.ts index 999badc17..7cd8c9564 100644 --- a/packages/start/src/server/handler.ts +++ b/packages/start/src/server/handler.ts @@ -1,5 +1,16 @@ +import { AsyncLocalStorage } from "node:async_hooks"; import middleware from "solid-start:middleware"; -import { defineHandler, getCookie, H3, type H3Event, iterable, redirect, setCookie } from "h3"; +import { + defineHandler, + getCookie, + H3, + type H3Event, + HTTPError, + iterable, + type Middleware, + redirect, + setCookie, +} from "h3"; import type { JSX } from "solid-js"; import { sharedConfig } from "solid-js"; import { getRequestEvent, renderToStream, renderToString } from "solid-js/web"; @@ -120,8 +131,111 @@ export function createBaseHandler( const app = new H3(); + app.use(captureUnhandledError(app)); app.use(handler); + return throwUnhandledErrors(app); +} + +/** + * A call of a handler's `fetch`: the handler's app, the request passed to the + * call, whether the app has started on it, and what the request's handler + * threw when that is not an answer (see `isAnswer`). Wrapped, so that a thrown + * `undefined` is still found. + */ +type FetchCall = { + app: H3; + request: Request; + started: boolean; + unhandled?: { thrown: unknown }; +}; + +/** + * The calls of every handler's `fetch`. One for the module: an instance that + * has run is only collected after `disable()`. + */ +const calls = new AsyncLocalStorage(); + +/** + * h3 answers whatever a handler throws, so `fetch` would never reject and the + * server hosting this handler would never see the error: it could neither + * report it nor answer it its own way. This keeps an unhandled error of the + * request passed to a `fetch` call away from h3 until that call throws it (see + * `throwUnhandledErrors`). The app's first start on that request owns the + * call; any other start is another way in, left as it was: + * `request()`, where h3 answers it, and an h3 app this one is mounted in, + * which gets it. + */ +function captureUnhandledError(app: H3): Middleware { + return async function captureUnhandledError(event, next) { + const call = calls.getStore(); + const own = call?.app === app && call.request === event.req && !call.started ? call : undefined; + if (own) own.started = true; + try { + return await next(); + } catch (thrown) { + if (!own || isAnswer(thrown)) throw thrown; + own.unhandled = { thrown }; + // Never sent: `fetch` throws instead. + return new Response(null, { status: 500 }); + } + }; +} + +/** h3's markers for "no route matched" and "already answered" (`Symbol.for`, as h3 makes them) */ +const kNotFound = Symbol.for("h3.notFound"); +const kHandled = Symbol.for("h3.handled"); + +/** + * Whether h3 answers a thrown value as the response the app asked for: a + * status thrown as a number, h3's not-found or handled marker, or an h3 + * `HTTPError` not marked as unhandled. + */ +function isAnswer(thrown: unknown): boolean { + return ( + typeof thrown === "number" || + thrown === kNotFound || + thrown === kHandled || + (isH3Error(thrown) && !thrown.unhandled) + ); +} + +/** + * Other libraries name their errors `HTTPError` too (ky's, got's). h3 takes + * such an error for its own (any of them before 2.0.1-rc.32, one with a + * `status` above 99 since), but needs its own error's `toJSON` to answer one. + */ +function isH3Error(thrown: unknown): thrown is HTTPError { + return HTTPError.isError(thrown) && typeof thrown.toJSON === "function"; +} + +/** + * The error the server hosting the handler gets: an `Error` as it was thrown, + * else an unhandled 500 caused by the thrown value, as h3's own `toError` + * makes it. An error named `HTTPError` that is not h3's (see `isH3Error`) is + * wrapped too, keeping its message and stack, so that the host does not take + * it for an h3 error and answer with its message. + */ +function toError(thrown: unknown): Error { + if (thrown instanceof Error && (thrown.name !== "HTTPError" || isH3Error(thrown))) return thrown; + const error = new HTTPError({ + status: 500, + unhandled: true, + message: thrown instanceof Error ? thrown.message : undefined, + }); + (error as { cause: unknown }).cause = thrown; + if (thrown instanceof Error) error.stack = thrown.stack; + return error; +} + +function throwUnhandledErrors(app: H3): H3 { + const fetch = app.fetch; + app.fetch = async request => { + const call: FetchCall = { app, request, started: false }; + const response = await calls.run(call, () => fetch(request)); + if (call.unhandled) throw toError(call.unhandled.thrown); + return response; + }; return app; } diff --git a/packages/start/src/server/types.ts b/packages/start/src/server/types.ts index 967125d43..dca6d8c72 100644 --- a/packages/start/src/server/types.ts +++ b/packages/start/src/server/types.ts @@ -12,6 +12,9 @@ import type { RequestEvent } from "solid-js/web"; * Referring to `H3` directly makes TypeScript emit a reference to h3's internal * `H3$1` class, which is not portable when h3 is nested inside * `node_modules/@solidjs/start/node_modules` (TS2742 / TS2883). + * + * Unlike `H3.fetch`, its `fetch` rejects with an error that h3 would answer + * as unhandled, for the server hosting it to report and answer. */ export interface StartHandler extends H3 {}