Skip to content

Conversation

@mcollina
Copy link
Member

Summary

  • Fixes WebSocket readyState incorrectly reverting from CLOSED (3) to CLOSING (2) after close event fires
  • Uses queueMicrotask() to defer onSocketClose() call, ensuring close event fires after state transition completes
  • Adds guard to prevent duplicate onSocketClose() calls

Test plan

Fixes: #4742

When calling close() on a CONNECTING WebSocket, the readyState was
correctly set to CLOSED during the close event handler, but incorrectly
reverted to CLOSING after the handler returned.

The issue was that failWebsocketConnection() called onSocketClose()
synchronously, which set readyState to CLOSED and fired the close event.
After returning, closeWebSocketConnection() would then overwrite the
state back to CLOSING.

Fix by using queueMicrotask() to defer onSocketClose(), ensuring the
close event fires after closeWebSocketConnection() completes. Also add
a guard to prevent duplicate onSocketClose() calls.

Fixes: #4742
@mcollina mcollina requested a review from KhafraDev January 22, 2026 08:45
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2026

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.26%. Comparing base (761fce9) to head (e8ba25f).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
lib/web/websocket/websocket.js 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4752      +/-   ##
==========================================
+ Coverage   93.22%   93.26%   +0.03%     
==========================================
  Files         109      109              
  Lines       34024    34010      -14     
==========================================
- Hits        31720    31718       -2     
+ Misses       2304     2292      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KhafraDev
Copy link
Member

#4745 (comment)

@mcollina
Copy link
Member Author

@KhafraDev that does not link directly to anything on my machine.

@KhafraDev
Copy link
Member

Not sure why, this is what I was linking to (domenic's comment):

I'm concerned this fix isn't quite following the spec. It adds some extra state transition guards and a microtask. Whereas, the proper per-spec fix would be to add a real task (not microtask) at a higher level. E.g. instead of a microtask just for the handler.onSocketClose() part of failWebsocketConnection, the entirety establishWebSocketConnection, closeWebSocketConnection, and whatever undici's equivalent of "when a WebSocket message has been received" is.

I suspect if that sort of fix is implemented, then no specific state transition patches will be necessary, since the state transitions will always be queued properly in the task queue.

@mcollina
Copy link
Member Author

I'm not convinced by that argument, unless needed by the spec.

The problem is that adding macroticks everywhere will necessarily slow things down and increase memory consumption.

cc @domenic

@domenic
Copy link
Contributor

domenic commented Jan 26, 2026

It is needed by the spec? The spec is very explicit!

Address review feedback to queue the close event as a task at a
higher level (in the handler) using setImmediate instead of
queueMicrotask in failWebsocketConnection.

This is more spec-compliant as the WebSocket spec uses "queue a task"
which maps to a macrotask (setImmediate) rather than a microtask.

Also updates the test to properly await the close event since it's
now queued asynchronously.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebSocket transitions back from CLOSED to CLOSING

4 participants