diff --git a/src/node/app.ts b/src/node/app.ts index ab185e40..a4e099c0 100644 --- a/src/node/app.ts +++ b/src/node/app.ts @@ -6,6 +6,7 @@ import http from "http" import * as httpolyglot from "httpolyglot" import * as util from "../common/util" import { DefaultedArgs } from "./cli" +import { isNodeJSErrnoException } from "./util" import { handleUpgrade } from "./wsRouter" /** @@ -33,21 +34,14 @@ export const createApp = async (args: DefaultedArgs): Promise<[Express, Express, resolve2() } server.on("error", (err) => { - if (!resolved) { - reject(err) - } else { - // Promise resolved earlier so this is an unrelated error. - util.logError(logger, "http server error", err) - } + handleServerError(resolved, err, reject) }) if (args.socket) { try { await fs.unlink(args.socket) - } catch (error) { - if (error.code !== "ENOENT") { - logger.error(error.message) - } + } catch (error: any) { + handleArgsSocketCatchError(error) } server.listen(args.socket, resolve) } else { @@ -69,10 +63,46 @@ export const createApp = async (args: DefaultedArgs): Promise<[Express, Express, export const ensureAddress = (server: http.Server): string => { const addr = server.address() if (!addr) { - throw new Error("server has no address") // NOTE@jsjoeio test this line + throw new Error("server has no address") } if (typeof addr !== "string") { return `http://${addr.address}:${addr.port}` } - return addr // NOTE@jsjoeio test this line + return addr +} + +/** + * Handles error events from the server. + * + * If the outlying Promise didn't resolve + * then we reject with the error. + * + * Otherwise, we log the error. + * + * We extracted into a function so that we could + * test this logic more easily. + */ +export const handleServerError = (resolved: boolean, err: Error, reject: (err: Error) => void) => { + // Promise didn't resolve earlier so this means it's an error + // that occurs before the server can successfully listen. + // Possibly triggered by listening on an invalid port or socket. + if (!resolved) { + reject(err) + } else { + // Promise resolved earlier so this is an unrelated error. + util.logError(logger, "http server error", err) + } +} + +/** + * Handles the error that occurs in the catch block + * after we try fs.unlink(args.socket). + * + * We extracted into a function so that we could + * test this logic more easily. + */ +export const handleArgsSocketCatchError = (error: any) => { + if (!isNodeJSErrnoException(error) || error.code !== "ENOENT") { + logger.error(error.message ? error.message : error) + } } diff --git a/src/node/util.ts b/src/node/util.ts index 61e410be..ce92a352 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -524,3 +524,12 @@ export function escapeHtml(unsafe: string): string { .replace(/"/g, """) .replace(/'/g, "'") } + +/** + * A helper function which returns a boolean indicating whether + * the given error is a NodeJS.ErrnoException by checking if + * it has a .code property. + */ +export function isNodeJSErrnoException(error: unknown): error is NodeJS.ErrnoException { + return error instanceof Error && (error as NodeJS.ErrnoException).code !== undefined +} diff --git a/test/unit/node/__snapshots__/app.test.ts.snap b/test/unit/node/__snapshots__/app.test.ts.snap new file mode 100644 index 00000000..10a0e6c6 --- /dev/null +++ b/test/unit/node/__snapshots__/app.test.ts.snap @@ -0,0 +1,3 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`handleServerError should log an error if resolved is true 1`] = `"Cannot read property 'handle' of undefined"`; diff --git a/test/unit/node/app.test.ts b/test/unit/node/app.test.ts index 41b5515a..d3fccc3c 100644 --- a/test/unit/node/app.test.ts +++ b/test/unit/node/app.test.ts @@ -1,6 +1,168 @@ +import { logger } from "@coder/logger" +import { promises, rmdirSync } from "fs" import * as http from "http" -import { ensureAddress } from "../../../src/node/app" -import { getAvailablePort } from "../../utils/helpers" +import * as https from "https" +import * as path from "path" +import { createApp, ensureAddress, handleArgsSocketCatchError, handleServerError } from "../../../src/node/app" +import { OptionalString, setDefaults } from "../../../src/node/cli" +import { generateCertificate } from "../../../src/node/util" +import { getAvailablePort, tmpdir } from "../../utils/helpers" + +describe("createApp", () => { + let spy: jest.SpyInstance + let unlinkSpy: jest.SpyInstance + let port: number + let tmpDirPath: string + let tmpFilePath: string + + beforeAll(async () => { + tmpDirPath = await tmpdir("unlink-socket") + tmpFilePath = path.join(tmpDirPath, "unlink-socket-file") + }) + + beforeEach(async () => { + spy = jest.spyOn(logger, "error") + // NOTE:@jsjoeio + // Be mindful when spying. + // You can't spy on fs functions if you do import * as fs + // You have to import individually, like we do here with promises + // then you can spy on those modules methods, like unlink. + // See: https://github.com/aelbore/esbuild-jest/issues/26#issuecomment-893763840 + unlinkSpy = jest.spyOn(promises, "unlink") + port = await getAvailablePort() + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + afterAll(() => { + jest.restoreAllMocks() + // Ensure directory was removed + rmdirSync(tmpDirPath, { recursive: true }) + }) + + it("should return an Express app, a WebSockets Express app and an http server", async () => { + const defaultArgs = await setDefaults({ + port, + _: [], + }) + const [app, wsApp, server] = await createApp(defaultArgs) + + // This doesn't check much, but it's a good sanity check + // to ensure we actually get back values from createApp + expect(app).not.toBeNull() + expect(wsApp).not.toBeNull() + expect(server).toBeInstanceOf(http.Server) + + // Cleanup + server.close() + }) + + it("should handle error events on the server", async () => { + const defaultArgs = await setDefaults({ + port, + _: [], + }) + + // This looks funky, but that's because createApp + // returns an array like [app, wsApp, server] + // We only need server which is at index 2 + // we do it this way so ESLint is happy that we're + // have no declared variables not being used + const app = await createApp(defaultArgs) + const server = app[2] + + const testError = new Error("Test error") + // We can easily test how the server handles errors + // By emitting an error event + // Ref: https://stackoverflow.com/a/33872506/3015595 + server.emit("error", testError) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledWith(`http server error: ${testError.message} ${testError.stack}`) + + // Cleanup + server.close() + }) + + it("should reject errors that happen before the server can listen", async () => { + // We listen on an invalid port + // causing the app to reject the Promise called at startup + const port = 2 + const defaultArgs = await setDefaults({ + port, + _: [], + }) + + async function masterBall() { + const app = await createApp(defaultArgs) + const server = app[2] + + const testError = new Error("Test error") + + server.emit("error", testError) + + // Cleanup + server.close() + } + + expect(() => masterBall()).rejects.toThrow(`listen EACCES: permission denied 127.0.0.1:${port}`) + }) + + it("should unlink a socket before listening on the socket", async () => { + await promises.writeFile(tmpFilePath, "") + const defaultArgs = await setDefaults({ + _: [], + socket: tmpFilePath, + }) + + const app = await createApp(defaultArgs) + const server = app[2] + + expect(unlinkSpy).toHaveBeenCalledTimes(1) + server.close() + }) + it("should catch errors thrown when unlinking a socket", async () => { + const tmpDir2 = await tmpdir("unlink-socket-error") + const tmpFile = path.join(tmpDir2, "unlink-socket-file") + // await promises.writeFile(tmpFile, "") + const socketPath = tmpFile + const defaultArgs = await setDefaults({ + _: [], + socket: socketPath, + }) + + const app = await createApp(defaultArgs) + const server = app[2] + + expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledWith(`ENOENT: no such file or directory, unlink '${socketPath}'`) + + server.close() + // Ensure directory was removed + rmdirSync(tmpDir2, { recursive: true }) + }) + + it("should create an https server if args.cert exists", async () => { + const testCertificate = await generateCertificate("localhost") + const cert = new OptionalString(testCertificate.cert) + const defaultArgs = await setDefaults({ + port, + cert, + _: [], + ["cert-key"]: testCertificate.certKey, + }) + const app = await createApp(defaultArgs) + const server = app[2] + + // This doesn't check much, but it's a good sanity check + // to ensure we actually get an https.Server + expect(server).toBeInstanceOf(https.Server) + + // Cleanup + server.close() + }) +}) describe("ensureAddress", () => { let mockServer: http.Server @@ -28,3 +190,107 @@ describe("ensureAddress", () => { expect(address).toBe(`http://localhost:8080`) }) }) + +describe("handleServerError", () => { + let spy: jest.SpyInstance + + beforeEach(() => { + spy = jest.spyOn(logger, "error") + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + afterAll(() => { + jest.restoreAllMocks() + }) + + it("should call reject if resolved is false", async () => { + const resolved = false + const reject = jest.fn((err: Error) => undefined) + const error = new Error("handleServerError Error") + + handleServerError(resolved, error, reject) + + expect(reject).toHaveBeenCalledTimes(1) + expect(reject).toHaveBeenCalledWith(error) + }) + + it("should log an error if resolved is true", async () => { + const resolved = true + const reject = jest.fn((err: Error) => undefined) + const error = new Error("handleServerError Error") + + handleServerError(resolved, error, reject) + + expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toThrowErrorMatchingSnapshot() + }) +}) + +describe("handleArgsSocketCatchError", () => { + let spy: jest.SpyInstance + + beforeEach(() => { + spy = jest.spyOn(logger, "error") + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + afterAll(() => { + jest.restoreAllMocks() + }) + + it("should log an error if its not an NodeJS.ErrnoException", () => { + const error = new Error() + + handleArgsSocketCatchError(error) + + expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledWith(error) + }) + + it("should log an error if its not an NodeJS.ErrnoException (and the error has a message)", () => { + const errorMessage = "handleArgsSocketCatchError Error" + const error = new Error(errorMessage) + + handleArgsSocketCatchError(error) + + expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledWith(errorMessage) + }) + + it("should not log an error if its a iNodeJS.ErrnoException", () => { + const error: NodeJS.ErrnoException = new Error() + error.code = "ENOENT" + + handleArgsSocketCatchError(error) + + expect(spy).toHaveBeenCalledTimes(0) + }) + + it("should log an error if the code is not ENOENT (and the error has a message)", () => { + const errorMessage = "no access" + const error: NodeJS.ErrnoException = new Error() + error.code = "EACCESS" + error.message = errorMessage + + handleArgsSocketCatchError(error) + + expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledWith(errorMessage) + }) + + it("should log an error if the code is not ENOENT", () => { + const error: NodeJS.ErrnoException = new Error() + error.code = "EACCESS" + + handleArgsSocketCatchError(error) + + expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledWith(error) + }) +})