Skip to content

node:dgram: throw ERR_SOCKET_DGRAM_NOT_RUNNING from socket methods after close() - #33024

Merged
Jarred-Sumner merged 2 commits into
mainfrom
farm/804e3919/dgram-not-running-after-close
Jun 29, 2026
Merged

Jarred-Sumner merged 2 commits into
mainfrom
farm/804e3919/dgram-not-running-after-close

Conversation

@robobun

@robobun robobun commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Any dgram.Socket method called after close() throws an internal, uncoded TypeError instead of Node's ERR_SOCKET_DGRAM_NOT_RUNNING:

const s = require("dgram").createSocket("udp4");
s.bind(0, () => {
  s.close();
  setTimeout(() => {
    try { s.address() } catch (e) { console.log(e.constructor.name, e.code, e.message) }
  }, 50);
});
// node: Error ERR_SOCKET_DGRAM_NOT_RUNNING "Not running"
// bun:  TypeError undefined "null is not an object (evaluating 'kStateSymbol')"

The same happens for send(), remoteAddress(), a second close(), and bind() after close. "The socket might already be closed" is a race UDP consumers handle by checking err.code === "ERR_SOCKET_DGRAM_NOT_RUNNING", so the uncoded TypeError (whose message also leaks an internal symbol name) turns a benign condition into an unclassifiable error.

Cause: close() sets state.handle = null, and the other methods dereference state.handle with no guard. dgram.ts already defines the same healthCheck() helper Node has (it throws ERR_SOCKET_DGRAM_NOT_RUNNING when the handle is gone), but nothing called it.

Fix: call healthCheck(this) from bind(), send(), close(), address() and remoteAddress(), at the same points Node's lib/dgram.js does. Also make Socket.prototype[Symbol.asyncDispose] check state.handle rather than state.handle.socket, again matching Node, so disposing an already-closed socket resolves instead of throwing the same TypeError.

Post-close behavior now matches Node v26.3.0 for every method Node guards:

after close() node bun before bun after
address() ERR_SOCKET_DGRAM_NOT_RUNNING uncoded TypeError ERR_SOCKET_DGRAM_NOT_RUNNING
remoteAddress() ERR_SOCKET_DGRAM_NOT_RUNNING uncoded TypeError ERR_SOCKET_DGRAM_NOT_RUNNING
send() ERR_SOCKET_DGRAM_NOT_RUNNING uncoded TypeError ERR_SOCKET_DGRAM_NOT_RUNNING
close() ERR_SOCKET_DGRAM_NOT_RUNNING uncoded TypeError ERR_SOCKET_DGRAM_NOT_RUNNING
bind() ERR_SOCKET_DGRAM_NOT_RUNNING error event with ERR_SOCKET_ALREADY_BOUND ERR_SOCKET_DGRAM_NOT_RUNNING
[Symbol.asyncDispose]() resolves rejects with TypeError resolves

Methods Node itself does not guard after close (connect(), setBroadcast(), setTTL(), the buffer-size accessors) are unchanged; the goal is parity with Node, and Node throws an unguarded error from those too.

How did you verify your code works?

  • New tests in test/js/bun/udp/dgram.test.ts (describe("after close()")). All 8 fail on current main and pass with this change.
  • The 29 existing tests in that file still pass.
  • Every vendored test/js/node/test/parallel/test-dgram-* file behaves identically before and after the change.

Note: the larger dgram sync in #32625 (draft) overlaps on address() and Symbol.asyncDispose, but does not cover send(), close(), remoteAddress() or bind() after close.

…ter close()

close() nulls the internal handle, so address(), remoteAddress(), send(),
a second close() and bind() on a closed socket dereferenced null and threw
an internal TypeError with no code. Call the existing healthCheck() guard
from each of those methods, at the same points Node's lib/dgram.js does,
and make Symbol.asyncDispose check the handle rather than handle.socket so
disposing an already-closed socket resolves instead of throwing.
@coderabbitai

coderabbitai Bot commented Jun 28, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 5 minutes and 48 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 01cab571-c222-4175-95d5-f6fb83cbd428

📥 Commits

Reviewing files that changed from the base of the PR and between 9787734 and ca73604.

📒 Files selected for processing (2)
  • src/js/node/dgram.ts
  • test/js/bun/udp/dgram.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jun 28, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:25 PM PT - Jun 28th, 2026

❌ @robobun, your commit ca73604 has 1 failures in Build #66563 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33024

That installs a local version of the PR into your bun-33024 executable, so you can run:

bun-33024 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — small, well-tested Node parity fix that wires up the existing healthCheck() helper.

Extended reasoning...

Overview

This PR touches two files: src/js/node/dgram.ts (5 added healthCheck(this) calls in bind(), send(), close(), address(), remoteAddress(), plus a one-token fix in Symbol.asyncDispose to check handle instead of handle.socket) and test/js/bun/udp/dgram.test.ts (8 new tests in a new describe("after close()") block). The healthCheck() helper already existed in the file and simply throws ERR_SOCKET_DGRAM_NOT_RUNNING when state.handle is null — it just wasn't being called anywhere. The placements mirror Node's lib/dgram.js exactly (e.g., in close() it's after the queue check, in send() it's after argument validation but before the implicit bind).

Security risks

None. This is purely defensive error-handling: it adds a null-guard that throws a properly-coded error in cases that previously crashed with an uncoded TypeError dereferencing null.socket. No new I/O, no auth/crypto/permissions, no user-controlled data flowing anywhere new.

Level of scrutiny

Low. The change is mechanical and additive — for any socket whose handle is non-null (the normal case), healthCheck() is a no-op, so the only observable behavior change is for already-closed sockets, where the old behavior was an internal crash. The Symbol.asyncDispose change is similarly safe: when handle exists but handle.socket doesn't (never-bound socket), it now proceeds into close(), which handles that case with optional chaining (state.handle.socket?.close()).

Other factors

  • Bug-hunting system found no issues.
  • 8 new targeted tests cover every guarded method plus the never-bound and asyncDispose edge cases; PR description states existing 29 tests and all vendored test-dgram-* Node parallel tests still pass.
  • Not covered by CODEOWNERS.
  • No prior reviewer comments to address.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — straightforward Node-compat fix wiring up the existing healthCheck() helper, with good test coverage.

Extended reasoning...

Overview

This PR touches two files: src/js/node/dgram.ts (~13 lines changed) and test/js/bun/udp/dgram.test.ts (8 new tests). The source change adds healthCheck(this) calls at the entry of bind(), send(), close(), address(), and remoteAddress(), and adjusts Symbol.asyncDispose to check state.handle instead of state.handle.socket. The healthCheck() helper already existed in the file (ported from Node's lib/dgram.js) but was never called; it simply throws ERR_SOCKET_DGRAM_NOT_RUNNING when the handle has been nulled by close().

Security risks

None. This is purely error-classification: it converts uncoded TypeError: null is not an object crashes into the coded ERR_SOCKET_DGRAM_NOT_RUNNING that Node throws in the same situations. No new I/O paths, no auth, no data exposure.

Level of scrutiny

Low. The change is mechanical and mirrors Node.js's lib/dgram.js call sites one-for-one (including the ordering in close() where the callback listener is registered before the health check, and in send() where the check comes after argument validation). The asyncDispose fix is a strict bug fix — the old state.handle.socket dereference would always TypeError once handle was null. The one intentional behavior change (bind() after close now throws synchronously instead of emitting ERR_SOCKET_ALREADY_BOUND) is documented in the PR table and matches Node.

Other factors

No CODEOWNERS cover these files. The bug-hunting system found nothing. Eight new regression tests exercise every changed method post-close, and the PR notes that all existing dgram.test.ts and vendored test-dgram-* parallel tests still pass. CI on the prior commit reported failures and was retriggered; a maintainer should confirm the retriggered build is green before merging, but the code itself is correct and self-contained.

@Jarred-Sumner
Jarred-Sumner merged commit d1e8adf into main Jun 29, 2026
72 of 73 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/804e3919/dgram-not-running-after-close branch June 29, 2026 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants