Nodejs Review

Reviewing Node.js

This skill is about the runtime, not the language. Type and syntax findings belong in review/typescript; what follows are defects that exist because the code runs on a single-threaded event loop with an async I/O layer, and would not exist in the same code running anywhere else.

The organising fact: one process, one thread for your code. Almost every check below is a way of occupying that thread, or of losing track of work scheduled on it.

The snippets are fragments cut down to the defect, not runnable programs.

Already covered - do not spend review on it

Rows 4 to 7 are the reason this skill exists: the runtime's most expensive mistakes have no diagnostic at all.

The checks that earn their place

Synchronous I/O on a request path

const cfg = fs.readFileSync(path);        // blocks every other request
const hash = crypto.pbkdf2Sync(pw, salt, 100000, 64, 'sha512');

When you see it: any *Sync call, execSync, or a CPU-bound loop inside a handler, a middleware, or a module that runs per request.

Ask: is this on the startup path or the request path? At startup, Sync is often the right choice and worth no comment. Per request, it stops the entire process — not just this request — and the latency lands on unrelated endpoints, which is why it is so hard to diagnose from the symptom.

A stream piped without backpressure

source.on('data', chunk => dest.write(chunk));   // ignores write() returning false

When you see it: a manual data/write loop instead of pipe or pipeline; res.write in a loop over a large result set.

Ask: what happens when the destination is slower than the source? write returning false means "buffer full"; ignoring it grows the buffer in memory until the process dies. pipeline() also propagates errors and destroys the streams, which a bare pipe does not.

An error listener that is not attached

const s = fs.createReadStream(p);
s.pipe(res);                                // no 's.on("error", ...)'

When you see it: a stream, a socket, or a child process without an error handler.

Ask: what happens on ENOENT? An error event with no listener is rethrown as an uncaught exception and takes the process down. pipeline(a, b, cb) is the form that routes it.

A listener added per request or per call

app.get('/x', (req, res) => {
  emitter.on('update', () => res.write('...'));   // never removed
});

When you see it: .on(...) inside a handler, a loop, or a reconnect path; process.on('unhandledRejection', ...) registered in a module loaded many times.

Ask: who removes this? Listeners accumulate on the emitter, hold the closure and everything it captured, and the only signal is a MaxListenersExceededWarning at 11 — which is a warning about the leak's size, not its existence. once or an explicit off in a finally.

process.exit() with output still buffered

console.log(report);
process.exit(0);          // stdout to a pipe is async: the write can be lost

When you see it: process.exit immediately after writing output, or inside a callback.

Ask: is stdout a TTY or a pipe here? To a terminal it is synchronous and the output appears; to a pipe or a file it is asynchronous, and exit truncates it. The bug shows up only when the CLI is used in a script — which is the case that matters. Setting process.exitCode and returning lets the loop drain.

spawn with a shell, or with unvalidated arguments

exec(`convert ${userFile} out.png`);        // a shell parses this
spawn('convert', [userFile, 'out.png']);    // no shell: argv is passed directly

When you see it: exec, execSync, or spawn(..., { shell: true }) with anything interpolated.

Ask: does a shell see this string? exec always spawns one, so ;, $() and backticks in the value are commands. spawn with an argv array and shell: false (the default) removes the whole category — and is also faster.

Unbounded concurrency in a fan-out

await Promise.all(urls.map(u => fetch(u)));   // 10,000 sockets at once

When you see it: Promise.all over an array whose length comes from data rather than from a constant.

Ask: how large can this get? Node will happily open every connection at once, exhausting file descriptors or the remote's rate limit. A concurrency limit makes the behaviour a decision instead of an accident. (Promise.all's partial-failure semantics are a separate concern — see review/typescript.)

A file handle or timer left open on the error path

const fh = await fs.promises.open(p);
const data = await parse(fh);        // throws: the handle is never closed
await fh.close();

When you see it: open, setInterval, a socket or a DB connection acquired without try/finally.

Ask: does every path release it? An open handle keeps the event loop alive, so the symptom is often a process that will not exit rather than an error. An un-unref'd setInterval does the same thing.

Module-scope state in a multi-instance deployment

let cache = {};                    // per process, not per app
export function get(k) { return cache[k]; }

When you see it: mutable module-level state used as a cache, a rate limiter, a session store, or a lock.

Ask: how many processes run this? Under cluster, PM2, or any horizontally scaled deployment, each has its own copy — so a rate limiter allows N times the limit and a lock locks nothing. It works perfectly on one dev machine.

Version- and deployment-dependent

What to say

Anchor the comment to the line, name the condition that triggers the defect, and say what breaks: "pbkdf2Sync here blocks the event loop for ~100 ms, so every concurrent request — including health checks — stalls behind it" beats "avoid sync APIs". If you cannot state the input that fails, it is a preference, not a defect.

Distinguish startup from request path before commenting on anything synchronous. That single distinction removes most false positives in Node review.