fix(worker): add thread safety to MessageChannel to avoid crashes#25806
fix(worker): add thread safety to MessageChannel to avoid crashes#25806xentobias wants to merge 15 commits into
MessageChannel to avoid crashes#25806Conversation
WalkthroughAdds per-instance locking to MessagePortChannel and MessagePortChannelRegistry, applies Locker scopes across methods to synchronize access, moves protector/ownership captures under lock and releases locks before callbacks, and adds two query helpers: Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (1)src/bun.js/bindings/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (5)📚 Learning: 2025-10-01T21:59:54.571ZApplied to files:
📚 Learning: 2025-12-23T06:50:31.577ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-11-24T18:37:47.899ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
🧬 Code graph analysis (3)src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (1)
🔇 Additional comments (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (1)
61-63: Implement missing methods with proper lock protection.The methods
hasAnyMessagesPendingOrInFlight()andbeingTransferredCount()are declared but have no implementations in the codebase. Both methods will need to access guarded members (m_pendingMessages,m_messageBatchesInFlight,m_pendingMessagePortTransfers) and must acquirem_lockbefore accessing them, following the pattern used by other methods in this class (e.g.,Locker locker { m_lock };).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 27ff6aa and 584a03130aa81a8f88a370cac7da5c4d39300e85.
📒 Files selected for processing (4)
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
🧰 Additional context used
📓 Path-based instructions (1)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cpp
🧠 Learnings (3)
📚 Learning: 2025-12-23T06:50:31.577Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:31.577Z
Learning: In Bun's C++ bindings, when returning an empty JSC::Identifier and a VM is accessible, prefer using vm.propertyNames->emptyIdentifier over constructing with JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier). The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns. Apply this guidance to src/bun.js/bindings/helpers.h and similar header files in the same bindings directory (i.e., any file that constructs an EmptyIdentifier).
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.hsrc/bun.js/bindings/webcore/MessagePortChannel.h
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.h
🧬 Code graph analysis (2)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(76-86)m_lock(82-82)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(76-86)m_lock(82-82)
🔇 Additional comments (20)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (1)
34-34: LGTM! Thread-safety infrastructure added correctly.The Lock include, member declaration, and WTF_GUARDED_BY_LOCK annotation properly establish the foundation for protecting m_openChannels from concurrent access across worker threads.
Also applies to: 61-62
src/bun.js/bindings/webcore/MessagePortChannel.h (1)
36-36: LGTM! All shared state properly annotated for thread-safety.The WTF_GUARDED_BY_LOCK annotations correctly identify all data members accessed concurrently from multiple worker threads. Note that m_ports remains unguarded, which is correct since it's immutable after construction.
Also applies to: 76-84
src/bun.js/bindings/webcore/MessagePortChannel.cpp (8)
41-53: Constructor safely initializes guarded members without lock.The constructor accesses guarded members (m_processes, m_entangledToProcessProtectors) without acquiring m_lock. This is correct because the object isn't visible to other threads until construction completes.
60-66: LGTM! Lock properly protects process identifier read.
73-85: LGTM! Lock properly protects state mutations during entanglement.
87-105: Excellent lock scope management prevents potential deadlock.The scoped block pattern (lines 95-104) ensures m_lock is released before
protectedThisgoes out of scope. If moving from m_entangledToProcessProtectors[i] releases the last reference to another MessagePortChannel, its destructor would call back into the registry. Releasing the lock first prevents holding two locks simultaneously and avoids potential deadlock.
107-124: LGTM! Lock properly protects port closure state.The
Ref protectedThis(line 114) is correctly created before acquiring the lock (line 116), ensuring the object stays alive during cleanup operations. Setting protectors to nullptr while holding the lock is safe because protectedThis keepsthisalive until after the lock is released.
126-142: LGTM! Lock properly protects message queue operations.
144-177: Excellent lock management for async callback pattern.The implementation correctly:
- Acquires lock (line 155) to safely capture state
- Swaps out pending messages and increments flight counter (lines 164-165)
- Moves protector to local variable (line 166)
- Releases lock before invoking callback (exits scope at line 167)
- Callback's completion handler re-acquires lock (line 173) to decrement counter
This pattern prevents holding the lock during callback execution while ensuring thread-safe state access.
179-191: LGTM! Lock properly protects single message retrieval.src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (10)
44-47: Destructor assertion safely accesses m_openChannels without lock.The ASSERT accesses m_openChannels without acquiring m_lock. This is correct because the destructor runs only after all references are gone, so no other threads can access the object.
57-67: LGTM! Lock properly protects channel registration.Holding m_lock for the entire operation is correct here since this method only manipulates the m_openChannels map without calling back into channel methods.
69-81: LGTM! Lock properly protects channel de-registration.Similar to messagePortChannelCreated, holding the lock throughout is appropriate for map-only operations.
83-100: Excellent deadlock prevention pattern.The lock-then-release pattern (lines 89-92) is critical:
- Acquire registry lock to safely get channel RefPtr
- Release registry lock immediately
- Call channel->entanglePortWithProcess() which acquires channel lock
This ensures the registry and channel locks are never held simultaneously, preventing deadlock. The RefPtr keeps the channel alive even if another thread removes it from the registry after the lock is released.
102-115: LGTM! Consistent lock-then-release pattern.
117-141: LGTM! Consistent lock-then-release pattern for port closure.
143-162: LGTM! Consistent lock-then-release pattern for message posting.
164-181: LGTM! Consistent lock-then-release pattern for message batch retrieval.
183-200: LGTM! Consistent lock-then-release pattern for single message retrieval.
202-208: LGTM! Lock properly protects channel lookup.Holding the lock only during the map lookup is correct. Callers must be aware that the returned pointer is not protected after the lock is released, but this appears to be a query method where the caller needs immediate access.
xentobias
force-pushed
the
claude/fix-bun-issue-1-B1Bix
branch
2 times, most recently
from
083dee7 to
54e3eda
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
144-177: LGTM: Message batch handling with proper lifecycle management.The lock discipline here is careful:
- Lock is held while swapping messages and incrementing
m_messageBatchesInFlight- Lock is released before invoking the callback (line 171)
- The completion handler passed to the callback locks again to decrement the counter
protectedThisis captured in the completion handler to keep the object alive until all messages are processedMinor observation: The early return at lines 158-160 invokes the callback while holding the lock. While this works (the callback is given empty data), releasing the lock before the callback would be more consistent with the pattern used for the non-empty case.
Optional: Release lock before callback in empty case
{ Locker locker { m_lock }; if (m_pendingMessages[i].isEmpty()) { - callback({}, [] {}); - return; + locker.unlockEarly(); } + else { + ASSERT(m_pendingMessageProtectors[i]); + + result.swap(m_pendingMessages[i]); + ++m_messageBatchesInFlight; + protectedThis = WTF::move(m_pendingMessageProtectors[i]); + } + } - ASSERT(m_pendingMessageProtectors[i]); - - result.swap(m_pendingMessages[i]); - ++m_messageBatchesInFlight; - protectedThis = WTF::move(m_pendingMessageProtectors[i]); + if (result.isEmpty()) { + callback({}, [] {}); + return; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 083dee7f5fd39c9285c7f0bdaca0254fb6975f7d and 54e3eda853e0122053f2d74bde39e241765820a4.
📒 Files selected for processing (4)
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
🧰 Additional context used
📓 Path-based instructions (1)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp
🧠 Learnings (4)
📚 Learning: 2025-12-23T06:50:31.577Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:31.577Z
Learning: In Bun's C++ bindings, when returning an empty JSC::Identifier and a VM is accessible, prefer using vm.propertyNames->emptyIdentifier over constructing with JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier). The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns. Apply this guidance to src/bun.js/bindings/helpers.h and similar header files in the same bindings directory (i.e., any file that constructs an EmptyIdentifier).
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h
🧬 Code graph analysis (4)
src/bun.js/bindings/webcore/MessagePortChannel.h (1)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (2)
MessagePortChannel(41-53)MessagePortChannel(55-58)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(76-86)m_lock(82-82)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(76-86)m_lock(82-82)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (3)
MessagePortChannel(42-88)m_lock(76-86)m_lock(82-82)
🔇 Additional comments (21)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (2)
34-34: LGTM: Lock include added.The include is necessary for the new
Lockmember and follows standard WTF patterns.
61-62: LGTM: Thread-safety guard added for channel registry.The
WTF_GUARDED_BY_LOCKannotation correctly documents thatm_openChannelsis protected bym_lock. The use ofWeakRef<MessagePortChannel>in the registry is appropriate, allowing channels to be destroyed independently while the registry maintains weak references.src/bun.js/bindings/webcore/MessagePortChannel.h (3)
36-36: LGTM: Lock include added.
61-63: LGTM: State query helpers added.Two new methods provide thread-safe access to internal state. Both acquire
m_lockin their implementations to safely read guarded members.
76-84: LGTM: Comprehensive thread-safety annotations.All mutable shared state is now guarded by
m_lock:
- Port state arrays (
m_isClosed,m_processes,m_entangledToProcessProtectors)- Message queues and transfers (
m_pendingMessages,m_pendingMessagePortTransfers,m_pendingMessageProtectors)- In-flight counter (
m_messageBatchesInFlight)The annotations correctly document the locking discipline enforced in the implementation file.
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (9)
57-67: LGTM: Registry mutation properly guarded.The lock correctly protects additions to
m_openChannelsduring channel creation.
69-81: LGTM: Registry cleanup properly synchronized.The lock protects verification and removal of channel entries from
m_openChannels.
83-100: LGTM: Correct deadlock avoidance pattern.The method correctly implements the two-phase locking strategy:
- Acquire registry lock → retrieve
RefPtr<MessagePortChannel>→ release registry lock- Call channel method (which acquires channel lock)
This prevents holding both locks simultaneously.
102-115: LGTM: Consistent deadlock avoidance pattern.Same two-phase locking strategy correctly applied.
117-141: LGTM: Lock pattern consistently applied.
143-162: LGTM: Message posting properly synchronized.
164-181: LGTM: Message retrieval properly synchronized.
183-200: LGTM: Single message retrieval properly synchronized.
202-208: LGTM: Channel lookup properly synchronized.This method uses a different pattern—it returns while holding the lock, unlike the other methods that release the registry lock before calling channel methods. This is acceptable since:
- It only performs a lookup and returns immediately
- The
WeakRefinm_openChannelsensures safe handling if the channel is destroyed- Callers must handle the possibility of a
nullptrreturnsrc/bun.js/bindings/webcore/MessagePortChannel.cpp (7)
60-66: LGTM: Process identifier read properly synchronized.Simple lock-read-return pattern for accessing guarded
m_processesarray.
73-85: LGTM: Port entanglement properly synchronized.The lock guards all related state mutations during port entanglement.
87-105: LGTM: Excellent lifecycle management to prevent destructor deadlock.The careful sequencing here is critical:
- Acquire lock
- Mutate state and move
m_entangledToProcessProtectors[i]to localprotectedThis- Release lock (scope exit at line 104)
- Allow
protectedThisto potentially trigger destructor (scope exit at line 105)This ensures the lock is not held when the destructor runs. The comment on lines 101-102 correctly documents this strategy.
107-124: LGTM: Port closure properly synchronized with lifecycle protection.The
protectedThiscreated before locking (line 114) ensures the object stays alive through the mutations, and the lock is released beforeprotectedThisgoes out of scope. Clean pattern.
126-142: LGTM: Message posting properly synchronized.The lock guards the message queue and the first-message protection logic correctly.
179-191: LGTM: Single message retrieval properly synchronized.Straightforward lock-protected read and modification of the message queue.
193-197: LGTM: Thread-safe state query.The const method correctly acquires the lock to safely read guarded state.
xentobias
force-pushed
the
claude/fix-bun-issue-1-B1Bix
branch
3 times, most recently
from
701674c to
a6a901c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
202-208: Potential use-after-free: raw pointer returned while lock is held, but released before caller uses it.
existingChannelContainingPortreturns a rawMessagePortChannel*after acquiring the lock. Once the lock is released (when function returns), another thread could destroy the channel before the caller uses the pointer.Consider returning a
RefPtr<MessagePortChannel>instead to ensure the channel stays alive for the caller.Proposed fix
-MessagePortChannel* MessagePortChannelRegistry::existingChannelContainingPort(const MessagePortIdentifier& port) +RefPtr<MessagePortChannel> MessagePortChannelRegistry::existingChannelContainingPort(const MessagePortIdentifier& port) { // ASSERT(isMainThread()); Locker locker { m_lock }; return m_openChannels.get(port); }Also update the declaration in
MessagePortChannelRegistry.h:- WEBCORE_EXPORT MessagePortChannel* existingChannelContainingPort(const MessagePortIdentifier&); + WEBCORE_EXPORT RefPtr<MessagePortChannel> existingChannelContainingPort(const MessagePortIdentifier&);src/bun.js/bindings/webcore/MessagePortChannel.cpp (2)
144-177: Callback invoked inside lock scope on early return - potential deadlock risk.On line 158,
callback({}, [] {})is invoked while still holdingm_lock. If the callback attempts to call back intoMessagePortChannelmethods, this could cause a deadlock.The later callback invocation (line 171) correctly happens outside the lock scope. Consider moving the early-return callback outside the lock for consistency.
Proposed fix
void MessagePortChannel::takeAllMessagesForPort(const MessagePortIdentifier& port, CompletionHandler<void(Vector<MessageWithMessagePorts>&&, CompletionHandler<void()>&&)>&& callback) { // LOG(MessagePorts, "MessagePortChannel %p taking all messages for port %s", this, port.logString().utf8().data()); ASSERT(port == m_ports[0] || port == m_ports[1]); size_t i = port == m_ports[0] ? 0 : 1; Vector<MessageWithMessagePorts> result; RefPtr<MessagePortChannel> protectedThis; + bool isEmpty = false; { Locker locker { m_lock }; - if (m_pendingMessages[i].isEmpty()) { - callback({}, [] {}); - return; - } + isEmpty = m_pendingMessages[i].isEmpty(); + if (isEmpty) + return callback({}, [] {}); ASSERT(m_pendingMessageProtectors[i]); result.swap(m_pendingMessages[i]); ++m_messageBatchesInFlight; protectedThis = WTF::move(m_pendingMessageProtectors[i]); }Wait, the return still happens inside scope. Better approach:
void MessagePortChannel::takeAllMessagesForPort(const MessagePortIdentifier& port, CompletionHandler<void(Vector<MessageWithMessagePorts>&&, CompletionHandler<void()>&&)>&& callback) { ASSERT(port == m_ports[0] || port == m_ports[1]); size_t i = port == m_ports[0] ? 0 : 1; Vector<MessageWithMessagePorts> result; RefPtr<MessagePortChannel> protectedThis; { Locker locker { m_lock }; if (m_pendingMessages[i].isEmpty()) { + locker.unlockEarly(); + callback({}, [] {}); return; } ASSERT(m_pendingMessageProtectors[i]); result.swap(m_pendingMessages[i]); ++m_messageBatchesInFlight; protectedThis = WTF::move(m_pendingMessageProtectors[i]); }Or restructure to avoid early return inside lock:
void MessagePortChannel::takeAllMessagesForPort(const MessagePortIdentifier& port, CompletionHandler<void(Vector<MessageWithMessagePorts>&&, CompletionHandler<void()>&&)>&& callback) { ASSERT(port == m_ports[0] || port == m_ports[1]); size_t i = port == m_ports[0] ? 0 : 1; Vector<MessageWithMessagePorts> result; RefPtr<MessagePortChannel> protectedThis; { Locker locker { m_lock }; - if (m_pendingMessages[i].isEmpty()) { - callback({}, [] {}); - return; - } - - ASSERT(m_pendingMessageProtectors[i]); - - result.swap(m_pendingMessages[i]); - ++m_messageBatchesInFlight; - protectedThis = WTF::move(m_pendingMessageProtectors[i]); + if (!m_pendingMessages[i].isEmpty()) { + ASSERT(m_pendingMessageProtectors[i]); + result.swap(m_pendingMessages[i]); + ++m_messageBatchesInFlight; + protectedThis = WTF::move(m_pendingMessageProtectors[i]); + } } + if (result.isEmpty()) { + callback({}, [] {}); + return; + } callback(WTF::move(result), [this, port, protectedThis = WTF::move(protectedThis)] { UNUSED_PARAM(port); Locker locker { m_lock }; --m_messageBatchesInFlight; }); }
179-191: Minor: Consider using takeFirst() for efficiency.
m_pendingMessages[i].first()followed byremoveAt(0)performs two operations.takeFirst()does this in one step and is more idiomatic for queues.Proposed fix
Locker locker { m_lock }; if (m_pendingMessages[i].isEmpty()) return std::nullopt; - auto message = m_pendingMessages[i].first(); - m_pendingMessages[i].removeAt(0); - return WTF::move(message); + return m_pendingMessages[i].takeFirst(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 701674c19eb210258bf3dde0ca7fcbd3c753804d and a6a901c713a8d1696bcc4fdffa40387981614879.
📒 Files selected for processing (4)
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
🧰 Additional context used
📓 Path-based instructions (1)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cpp
🧠 Learnings (4)
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
📚 Learning: 2025-12-23T06:50:31.577Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:31.577Z
Learning: In Bun's C++ bindings, when returning an empty JSC::Identifier and a VM is accessible, prefer using vm.propertyNames->emptyIdentifier over constructing with JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier). The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns. Apply this guidance to src/bun.js/bindings/helpers.h and similar header files in the same bindings directory (i.e., any file that constructs an EmptyIdentifier).
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
🧬 Code graph analysis (3)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(76-86)m_lock(82-82)
src/bun.js/bindings/webcore/MessagePortChannel.h (1)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (4)
beingTransferredCount(199-203)beingTransferredCount(199-199)MessagePortChannel(41-53)MessagePortChannel(55-58)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(76-86)m_lock(82-82)
🔇 Additional comments (21)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (8)
57-67: LGTM: Registry creation locking is correct.The lock properly guards both
m_openChannels.addoperations, ensuring thread-safe registration of both ports.
69-81: LGTM: Registry destruction locking is correct.The lock guards the assertions and removal operations atomically, preventing races during channel cleanup.
83-100: LGTM: Proper lock-release-before-callback pattern.Acquiring the
RefPtr<MessagePortChannel>under lock, then releasing the registry lock before callingchannel->entanglePortWithProcess()correctly avoids potential deadlocks when the channel acquires its own lock.
102-115: LGTM: Consistent deadlock-avoidance pattern.Same pattern as
didEntangleLocalToRemote- acquire channel reference under lock, release, then call channel method.
117-141: LGTM: closePort follows the same safe pattern.Lock scope correctly limited to registry access, with channel method called after lock release.
143-162: LGTM: didPostMessageToRemote correctly synchronized.The pattern is consistent with other methods - registry lock released before calling channel->postMessageToRemote.
164-181: LGTM: takeAllMessagesForPort correctly synchronized.The lock-release-before-callback pattern is properly applied.
183-200: LGTM: tryTakeMessageForPort correctly synchronized.Consistent deadlock-avoidance pattern applied.
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (2)
34-34: LGTM: Correct include for Lock.
61-62: LGTM: Thread-safety annotations correctly applied.The
WTF_GUARDED_BY_LOCK(m_lock)annotation onm_openChannelsenables compile-time thread-safety analysis. Themutablequalifier onm_lockis appropriate for potential future const methods.src/bun.js/bindings/webcore/MessagePortChannel.h (3)
36-36: LGTM: Correct include for Lock.
63-63: LGTM: const-correctness applied.Making
beingTransferredCount()const is correct since it only reads state under lock.
75-85: LGTM: Thread-safety annotations correctly cover all mutable shared state.The unguarded members (
m_portsandm_registry) are correctly excluded - they are set at construction time and never modified afterward. All mutable state accessed from multiple threads is properly annotated withWTF_GUARDED_BY_LOCK(m_lock).src/bun.js/bindings/webcore/MessagePortChannel.cpp (8)
41-53: Verify: Constructor registers channel before construction completes.The constructor accesses guarded members (
m_processes,m_entangledToProcessProtectors) without holding the lock, which is correct since the object isn't shared yet. However,m_registry.messagePortChannelCreated(*this)is called at the end, making the channel visible to other threads while still in the constructor.Since
messagePortChannelCreatedacquires the registry lock and only adds the channel tom_openChannels, and the channel's guarded state is fully initialized before this call, this should be safe. But consider whether any code paths could access the channel's methods before the constructor stack frame unwinds.
60-66: LGTM: processForPort correctly synchronized.Lock is acquired before reading
m_processes[i]. The ASSERT on line 62 only accesses immutablem_ports, so it's correctly placed before the lock.
68-71: LGTM: includesPort doesn't need locking.This method only reads
m_portswhich is immutable after construction.
73-85: LGTM: entanglePortWithProcess correctly synchronized.Lock acquired before accessing all guarded members. The ASSERT on line 75 safely accesses immutable
m_ports.
87-105: LGTM: disentanglePort uses safe lock-release-before-destructor pattern.Capturing
protectedThisunder lock and moving it out ensures the lock is released before the potential destructor runs whenprotectedThisgoes out of scope.
107-124: LGTM: closePort correctly synchronized with protectedThis pattern.Creating
Ref protectedThisbefore acquiring the lock ensures the object stays alive. All guarded member modifications happen under lock.
126-142: LGTM: postMessageToRemote correctly synchronized.Lock held for the entire critical section involving
m_pendingMessagesandm_pendingMessageProtectors.
193-203: LGTM: Query methods correctly synchronized.Both
hasAnyMessagesPendingOrInFlight()andbeingTransferredCount()acquire the lock before reading guarded members. Theconstqualifier is correctly applied since they only read state.
## Problem MessageChannel would crash with a segmentation fault when workers exchanged many messages rapidly (e.g., using setInterval). The issue manifested in two ways: 1. Segmentation fault at address 0x30 during high-volume message passing 2. Assertion failure: ASSERTION FAILED: m_openChannels.get(channel.port1()) == &channel ## Root Cause Both MessagePortChannel and MessagePortChannelRegistry had critical thread safety issues. These classes were being accessed from multiple worker threads simultaneously without any synchronization: **MessagePortChannel** - No locks protecting: - m_pendingMessages[2]: Vector operations (append, swap, removeAt) - m_processes[2]: Process identifier tracking - m_entangledToProcessProtectors[2]: Reference counting - m_messageBatchesInFlight: Message counter **MessagePortChannelRegistry** - Global singleton with no locks protecting: - m_openChannels: HashMap accessed by all workers for channel lookup When Worker1 and Worker2 both called postMessageToRemote() or takeAllMessagesForPort() concurrently, race conditions on Vector and HashMap operations led to memory corruption and crashes. ## Solution ### MessagePortChannel Thread Safety - Added Lock m_lock member variable - Protected all shared state with WTF_GUARDED_BY_LOCK(m_lock) annotations - Added Locker guards in 7 methods: - processForPort() - entanglePortWithProcess() - disentanglePort() - closePort() - postMessageToRemote() - takeAllMessagesForPort() - tryTakeMessageForPort() ### MessagePortChannelRegistry Thread Safety - Added Lock m_lock member variable - Protected m_openChannels HashMap with WTF_GUARDED_BY_LOCK(m_lock) - Added Locker guards in 9 methods: - messagePortChannelCreated() - messagePortChannelDestroyed() - didEntangleLocalToRemote() - didDisentangleMessagePort() - didCloseMessagePort() - didPostMessageToRemote() - takeAllMessagesForPort() - tryTakeMessageForPort() - existingChannelContainingPort() ### Deadlock Prevention To avoid deadlocks (since both Registry and Channel have locks), the pattern used is: 1. Grab a RefPtr<MessagePortChannel> while holding the registry lock 2. Release the registry lock 3. Call the channel's methods (which acquire the channel's lock) This ensures we never hold two locks simultaneously while maintaining thread safety. ## Files Modified - src/bun.js/bindings/webcore/MessagePortChannel.h - src/bun.js/bindings/webcore/MessagePortChannel.cpp - src/bun.js/bindings/webcore/MessagePortChannelRegistry.h - src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp Fixes: oven-sh#25805 Fixes: oven-sh#16186
xentobias
force-pushed
the
claude/fix-bun-issue-1-B1Bix
branch
from
a6a901c to
bc5c1e4
Compare
The channel might have already been removed from m_openChannels by the time the destructor is called (e.g., if didCloseMessagePort was called first). Changed the ASSERT to a conditional check to handle this case gracefully. Also fixed messagePortChannelCreated to clean up stale WeakRef entries before adding new channels. When a channel is destroyed, the WeakRef becomes null but the map entry may still exist, so we need to remove these before adding new entries with the same identifiers.
xentobias
force-pushed
the
claude/fix-bun-issue-1-B1Bix
branch
from
6402b5a to
a674cd5
Compare
This reverts commit a674cd5.
…essaging - Handle race condition in messagePortChannelDestroyed where channel may already be removed when both ports close from different threads - Clean up stale WeakRef entries in messagePortChannelCreated - Implement missing hasAnyMessagesPendingOrInFlight() and beingTransferredCount() - Make m_lock mutable for const method access
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (2)
44-47: Consider acquiring lock in destructor for consistency.The destructor asserts
m_openChannels.isEmpty()without holdingm_lock. While this is likely safe if shutdown is properly ordered (ensuring no concurrent operations), acquiring the lock would be more defensive and consistent with the thread-safety pattern applied elsewhere.Optional: Add lock acquisition in destructor
MessagePortChannelRegistry::~MessagePortChannelRegistry() { + Locker locker { m_lock }; ASSERT(m_openChannels.isEmpty()); }
221-227: Return type inconsistency:existingChannelContainingPortshould returnRefPtrlike similar methods in the same class.
existingChannelContainingPortreturns a rawMessagePortChannel*fromWeakRef::get()while holding the lock. Once the lock is released, the channel could be destroyed by another thread before the caller uses the pointer. All other methods in this class (didPostMessageToRemote,takeAllMessagesForPort,tryTakeMessageForPort) correctly useRefPtr<MessagePortChannel>assignments within the lock scope, which increments the reference count and ensures the channel stays alive after the lock is released. Change the return type toRefPtr<MessagePortChannel>to match this pattern.Note: No callers of this function were found in the codebase. If this is dead code, consider removing it; if it's part of the public API (WEBCORE_EXPORT), ensure external callers handle the raw pointer safely.
src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
179-191: LGTM: Lock correctly protects message queue access.The locking correctly protects concurrent access to
m_pendingMessages.Note: The
removeAt(0)operation is O(n) forVector, which could be a performance concern for high-volume messaging. Consider usingDequeif this becomes a bottleneck, though this is pre-existing behavior and outside the scope of this fix.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between a6a901c713a8d1696bcc4fdffa40387981614879 and 343af4d.
📒 Files selected for processing (4)
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
🧰 Additional context used
📓 Path-based instructions (1)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cpp
🧠 Learnings (5)
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.hsrc/bun.js/bindings/webcore/MessagePortChannel.cpp
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cpp
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp
📚 Learning: 2025-12-23T06:50:31.577Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:31.577Z
Learning: In Bun's C++ bindings, when returning an empty JSC::Identifier and a VM is accessible, prefer using vm.propertyNames->emptyIdentifier over constructing with JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier). The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns. Apply this guidance to src/bun.js/bindings/helpers.h and similar header files in the same bindings directory (i.e., any file that constructs an EmptyIdentifier).
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.hsrc/bun.js/bindings/webcore/MessagePortChannel.h
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.cpp
🧬 Code graph analysis (3)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (3)
MessagePortChannel(42-69)m_lock(76-86)m_lock(82-82)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(76-86)m_lock(82-82)
src/bun.js/bindings/webcore/MessagePortChannel.h (1)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (4)
beingTransferredCount(199-203)beingTransferredCount(199-199)MessagePortChannel(41-53)MessagePortChannel(55-58)
🔇 Additional comments (10)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (1)
61-62: LGTM: Thread-safety annotations correctly applied to registry.The lock and guarded annotation are properly added to protect
m_openChannelsfrom concurrent access. The use ofWeakRef<MessagePortChannel>is appropriate here to avoid preventing channel destruction while still allowing lookup.src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (3)
57-78: LGTM: Proper lock acquisition with defensive stale entry cleanup.The locking and stale WeakRef cleanup before insertion is correct. This handles the case where a channel was destroyed but the map entry wasn't removed (WeakRef becomes null).
80-100: LGTM: Defensive removal logic handles concurrent destruction.The conditional removal (only if entry is null/stale or matches this channel) correctly handles the case where both ports are closed in quick succession from different threads.
102-119: LGTM: Correct deadlock avoidance pattern.Acquiring
RefPtr<MessagePortChannel>while holding the registry lock, then releasing the lock before calling channel methods, correctly avoids potential deadlock between the registry lock and channel lock.src/bun.js/bindings/webcore/MessagePortChannel.h (1)
75-85: LGTM: Thread-safety annotations correctly applied to channel state.All mutable shared state is properly guarded by
m_lock. Themutablekeyword on the lock is correct for use inconstmethods likehasAnyMessagesPendingOrInFlight()andbeingTransferredCount(). Them_portsarray is correctly left unguarded as it's immutable after construction.src/bun.js/bindings/webcore/MessagePortChannel.cpp (5)
41-53: LGTM: Constructor correctly initializes state without locking.Locking isn't needed in the constructor because the object isn't yet shared with other threads. The
relaxAdoptionRequirement()call and subsequent initialization are safe.
87-105: LGTM: Correct ownership transfer to avoid destruction under lock.The pattern of capturing
m_entangledToProcessProtectors[i]intoprotectedThisunder lock, then releasing the lock beforeprotectedThisgoes out of scope, correctly ensures that if this was the last reference, destruction (which callsmessagePortChannelDestroyed) happens without holdingm_lock.
107-124: LGTM: Proper lifetime management with lock protection.Creating
Ref protectedThisbefore acquiring the lock ensures the channel survives through the method even when internal protector refs are cleared. All guarded state modifications are correctly performed under the lock.
171-177: LGTM: Callback correctly invoked outside lock with proper lifetime management.The normal path correctly releases the lock before invoking the callback. The lambda captures
protectedThisto ensure the channel remains alive, and safely re-acquiresm_lockwhen decrementingm_messageBatchesInFlight.
193-203: LGTM: New query methods correctly synchronized.Both
hasAnyMessagesPendingOrInFlight()andbeingTransferredCount()correctly acquire the lock before reading guarded state. Theconstqualification is appropriate since they only read state.
… outside lock Move the empty-case callback invocation outside the Locker scope to prevent deadlocks if the callback acquires the same or other locks.
xentobias
changed the title
MessageChannel to avoid crashes
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Thank you for this. Can you add tests that were failing on main/previous versions of Bun that now pass in this branch?
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
221-227: Critical race condition: raw pointer returned without reference counting after lock release.This method returns a raw
MessagePortChannel*pointer to data protected by a lock. After the lock is released, another thread could destroy the channel, causing a use-after-free if the caller dereferences the pointer. All other similar methods in this file correctly capture the result into aRefPtrwhile holding the lock, which increments the reference count and prevents the object from being destroyed.Change the method to return
RefPtr<MessagePortChannel>instead ofMessagePortChannel*:Fix: Return RefPtr to prevent use-after-free
-MessagePortChannel* MessagePortChannelRegistry::existingChannelContainingPort(const MessagePortIdentifier& port) +RefPtr<MessagePortChannel> MessagePortChannelRegistry::existingChannelContainingPort(const MessagePortIdentifier& port) { // ASSERT(isMainThread()); Locker locker { m_lock }; return m_openChannels.get(port); }Update the header declaration:
-MessagePortChannel* existingChannelContainingPort(const MessagePortIdentifier&); +RefPtr<MessagePortChannel> existingChannelContainingPort(const MessagePortIdentifier&);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(76-86)m_lock(82-82)
🔇 Additional comments (3)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (3)
57-78: LGTM! Proper locking and cleanup of stale entries.The addition of
Locker locker { m_lock }correctly guards access tom_openChannels. The cleanup logic that removes stale WeakRef entries (lines 63-71) before adding new channels is appropriate and prevents assertion failures when port identifiers are reused after a channel is destroyed.
80-100: LGTM! Defensive checks are appropriate for multi-threaded scenarios.The replacement of aggressive assertions with defensive checks (lines 86-97) correctly handles the case where channel entries might have been removed or nullified by concurrent threads. The logic only removes entries that either point to this channel or are already null/stale, preventing crashes from race conditions.
102-219: LGTM! Excellent deadlock avoidance pattern.The consistent pattern across all these methods (lines 107-111, 126-130, 142-146, 169-173, 188-192, 209-213) correctly avoids deadlocks by:
- Acquiring the registry lock in a narrow scope
- Capturing a
RefPtr<MessagePortChannel>to keep the channel alive- Releasing the registry lock before calling channel methods (which acquire their own lock)
This prevents holding two locks simultaneously while ensuring the channel remains valid during use.
…workers This commit introduces a new fixture script and corresponding tests to validate the thread safety of MessageChannel communication between workers. The fixture simulates rapid message passing to reproduce a crash scenario from issue oven-sh#25805, ensuring that the system remains stable under stress. The tests confirm that no segmentation faults occur during execution, validating the effectiveness of the implemented fixes.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @test/js/web/workers/message-channel-thread-safety-fixture.js:
- Around line 23-26: Remove the unused commented-out for-loop block and its
commented postMessage calls to tidy the test; specifically delete the commented
lines containing "for (let i = 0; i < 100000; i++) {",
"worker1.postMessage(\"PING\");", and "worker2.postMessage(\"PING\");" so the
file only contains the active while-loop approach that uses worker1 and worker2.
- Line 29: The inline comment for the duration constant is inconsistent with its
value: update the comment or the value so they match; specifically, in the
declaration of the duration constant named duration (currently set to 500),
either change the comment to "Run for 500ms" or set duration to 2000 if the
intention was 2 seconds—make sure the comment and the duration variable agree.
In @test/js/web/workers/message-channel-thread-safety.test.ts:
- Around line 21-23: Read proc.stdout and proc.stderr (e.g., await new
Response(proc.stdout).text() and await new Response(proc.stderr).text()) before
awaiting proc.exited to ensure error output is captured; move the lines that
compute stdout and stderr above the line that awaits exitCode (const exitCode =
await proc.exited) so the test reads the streams prior to waiting for process
termination.
- Line 34: Remove the explicit 60s timeout argument passed to the test function
(the call that currently ends with "}, 60000);"); update the test invocation to
drop the trailing timeout parameter so the call ends normally (e.g., "});"),
leaving the test body and its async function intact; ensure no other references
to that numeric timeout remain in the same test block.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/js/web/workers/message-channel-thread-safety-fixture.jstest/js/web/workers/message-channel-thread-safety.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/web/workers/message-channel-thread-safety.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/web/workers/message-channel-thread-safety.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/web/workers/message-channel-thread-safety.test.ts
🧠 Learnings (14)
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.tstest/js/web/workers/message-channel-thread-safety-fixture.js
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process
Applied to files:
test/js/web/workers/message-channel-thread-safety.test.ts
🧬 Code graph analysis (1)
test/js/web/workers/message-channel-thread-safety.test.ts (2)
test/harness.ts (1)
bunExe(102-105)packages/bun-inspector-protocol/src/protocol/jsc/index.d.ts (1)
Response(2793-2806)
This comment was marked as outdated.
This comment was marked as outdated.
…lated registry methods This commit modifies the MessagePortChannel class to inherit from ThreadSafeRefCounted and updates the MessagePortChannelRegistry to utilize ThreadSafeWeakPtr for managing open channels. Additionally, it removes the obsolete relaxAdoptionRequirement call and adjusts related comments and method signatures to reflect the new reference management strategy.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
44-47: Potential race condition in destructor assertion.The
ASSERT(m_openChannels.isEmpty())on line 46 readsm_openChannelswithout holdingm_lock. While this is typically safe because the destructor should only run when no other threads reference the registry, consider adding locking for consistency and to satisfy static analysis tools.Suggested fix
MessagePortChannelRegistry::~MessagePortChannelRegistry() { + Locker locker { m_lock }; ASSERT(m_openChannels.isEmpty()); }src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
188-194: Consider usingtakeFirst()for efficiency.The current implementation copies the first element with
.first()then removes it with.removeAt(0). UsingtakeFirst()would be more efficient as it moves the element out in a single operation.Suggested improvement
Locker locker { m_lock }; if (m_pendingMessages[i].isEmpty()) return std::nullopt; - auto message = m_pendingMessages[i].first(); - m_pendingMessages[i].removeAt(0); - return WTF::move(message); + return m_pendingMessages[i].takeFirst(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
🧰 Additional context used
📓 Path-based instructions (1)
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cpp
🧠 Learnings (5)
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cpp
📚 Learning: 2025-12-23T06:50:31.577Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:31.577Z
Learning: In Bun's C++ bindings, when returning an empty JSC::Identifier and a VM is accessible, prefer using vm.propertyNames->emptyIdentifier over constructing with JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier). The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns. Apply this guidance to src/bun.js/bindings/helpers.h and similar header files in the same bindings directory (i.e., any file that constructs an EmptyIdentifier).
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.hsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/MessagePortChannelRegistry.h
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/webcore/MessagePortChannel.cpp
🧬 Code graph analysis (3)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(77-87)m_lock(83-83)
src/bun.js/bindings/webcore/MessagePortChannel.cpp (1)
src/bun.js/bindings/webcore/MessagePortChannel.h (2)
m_lock(77-87)m_lock(83-83)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (1)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (6)
existingChannelContainingPort(221-227)existingChannelContainingPort(221-221)messagePortChannelCreated(57-78)messagePortChannelCreated(57-57)messagePortChannelDestroyed(80-100)messagePortChannelDestroyed(80-80)
🔇 Additional comments (13)
src/bun.js/bindings/webcore/MessagePortChannelRegistry.h (3)
34-35: LGTM! Appropriate thread-safety headers added.The inclusion of
<wtf/Lock.h>and<wtf/ThreadSafeWeakPtr.h>correctly supports the thread-safe channel management introduced in this PR.
56-56: Good change: Return type changed toRefPtr<MessagePortChannel>.Returning
RefPtrinstead of raw pointer ensures the caller holds a strong reference to the channel, preventing use-after-free when the channel is accessed after the registry lock is released.
62-63: Thread-safe storage pattern correctly implemented.Using
ThreadSafeWeakPtr<MessagePortChannel>withWTF_GUARDED_BY_LOCK(m_lock)annotation is the correct approach for cross-thread channel access. The weak pointer allows channels to be destroyed independently while the lock ensures thread-safe map operations.src/bun.js/bindings/webcore/MessagePortChannel.h (2)
35-37: Correct thread-safety primitives for cross-thread channel access.Switching to
ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtras the base class is appropriate sinceMessagePortChannelinstances are now accessed from multiple worker threads. The corresponding header includes support the new ownership model.Also applies to: 43-43
77-85: Comprehensive guarding of mutable shared state.All mutable members that can be accessed concurrently are properly annotated with
WTF_GUARDED_BY_LOCK(m_lock). Themutablequalifier onm_lockcorrectly enables locking inconstmethods likehasAnyMessagesPendingOrInFlight()andbeingTransferredCount().Note:
m_ports(line 76) andm_registry(line 86) are not guarded, which is correct since they are set once in the constructor and never modified thereafter.src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp (4)
61-77: Good defensive cleanup of stale weak pointer entries.The pattern of checking for stale
ThreadSafeWeakPtrentries before adding new channels handles the case where a channel was destroyed but the map entry wasn't yet removed. This prevents assertion failures when port identifiers are reused.
84-97: Defensive removal pattern correctly handles race conditions.The check
if (!existingChannel1 || existingChannel1.get() == &channel)properly handles the case where another thread already removed/replaced the entry, avoiding incorrect removal of a different channel's entry.
107-118: Correct deadlock avoidance: release registry lock before calling channel methods.The pattern of acquiring
RefPtr<MessagePortChannel>underm_lock, then releasing the lock before callingchannel->entanglePortWithProcess()prevents potential deadlock since channel methods acquireMessagePortChannel::m_lock.
221-227: LGTM! Thread-safe channel lookup.Returns a
RefPtrwhich ensures the caller has a strong reference, and the lookup is properly guarded bym_lock.src/bun.js/bindings/webcore/MessagePortChannel.cpp (4)
41-51: Constructor correctly initializes without locking.Accessing guarded members in the constructor without holding
m_lockis safe because the object isn't yet visible to other threads. The channel is only added to the registry at line 50, after initialization is complete.
92-102: Proper lock release before potential destructor.Capturing
protectedThisinside the lock scope and letting it destruct after the lock is released ensures the lock isn't held during potential destructor execution. This avoids deadlock if the destructor needs to access locked resources.
149-180: Correct callback invocation pattern.The callback is invoked outside the lock (lines 167-171 and 175-180) to prevent deadlock. The inner completion handler correctly re-acquires
m_lockwhen decrementingm_messageBatchesInFlight. TheprotectedThiscapture ensures the channel stays alive during async callback execution.
197-207: LGTM! New query methods correctly implemented.Both
hasAnyMessagesPendingOrInFlight()andbeingTransferredCount()properly acquirem_lockbefore reading guarded members. Theconstqualifier combined withmutable Lockis the correct pattern for read-only thread-safe accessors.
|
@Jarred-Sumner As requested, I’ve added a test that puts load on the |
|
ping @Jarred-Sumner |
Code ReviewNewest first 🟡 b99b0 — 3 minor issue(s) found
Powered by Claude Code Review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| expect(stderr).not.toContain("Bun has crashed"); | ||
| expect(exitCode).toBe(0); | ||
| expect(stdout).toContain("SUCCESS"); | ||
| }, 60000); |
There was a problem hiding this comment.
🟡 Minor: Nit: explicit test timeout of 60000ms violates the project's testing guidelines. Per test/CLAUDE.md line 120: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." Remove the timeout parameter.
| }, 60000); | |
| }); |
Why this is a problem
Issue
The test at line 34 of test/js/web/workers/message-channel-thread-safety.test.ts passes an explicit timeout of 60000 (60 seconds) as the second argument to the test() function:
}, 60000);This directly violates a CRITICAL rule in the project's testing guidelines.
Rule Violated
The test/CLAUDE.md file states at line 120, under the "No timeouts" section:
CRITICAL: Do not set a timeout on tests. Bun already has timeouts.
This rule exists because Bun's test runner has its own built-in timeout management. Adding explicit timeouts can mask test failures, create inconsistencies with the framework's timeout behavior, and lead to flaky tests that behave differently across environments.
How to Fix
Remove the 60000 timeout parameter from the test() call on line 34. Change:
expect(stdout).toContain("SUCCESS");
}, 60000);To:
expect(stdout).toContain("SUCCESS");
});
| test("MessageChannel between workers does not crash with rapid messages", async () => { | ||
| const fixturePath = path.join(import.meta.dir, "message-channel-thread-safety-fixture.js"); | ||
|
|
||
| const proc = Bun.spawn({ |
There was a problem hiding this comment.
🟡 Minor: Per the project's test style guide, spawned processes should use await using for proper resource cleanup. Without it, if an assertion fails before proc.exited resolves, the spawned process handle may leak.
| const proc = Bun.spawn({ | |
| await using proc = Bun.spawn({ |
Why this is a problem
Issue
The test spawns a Bun process using const proc = Bun.spawn(...) on line 14, but the project's testing conventions require await using proc = Bun.spawn(...) for proper resource cleanup via the explicit resource management pattern.
CLAUDE.md Rule Violated
The test/CLAUDE.md file documents this pattern in multiple places. Both code examples for spawning processes (lines 36-40 and 74-78) consistently use await using proc = Bun.spawn(...). Additionally, line 100 explicitly states:
Generally,
await usingorusingis a good idea to ensure proper resource cleanup. This works in most Bun APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc.
The root CLAUDE.md also shows this pattern in its test examples, using await using proc = Bun.spawn({...}) as the canonical form.
Impact
Without await using, if any assertion fails before proc.exited resolves (for example, the expect(stderr).not.toContain("Segmentation fault") check on line 30), the spawned process handle may not be properly cleaned up. The process could remain running as an orphan, potentially leading to resource leaks during test runs.
Suggested Fix
Change line 14 from:
const proc = Bun.spawn({to:
await using proc = Bun.spawn({This single-line change brings the test in line with the project's established testing conventions and ensures the process handle is properly disposed of regardless of whether the test passes or fails.
| expect(stderr).not.toContain("Segmentation fault"); | ||
| expect(stderr).not.toContain("Bun has crashed"); | ||
| expect(exitCode).toBe(0); | ||
| expect(stdout).toContain("SUCCESS"); |
There was a problem hiding this comment.
🟡 Minor: Nit: expect(stdout).toContain("SUCCESS") on line 33 comes after expect(exitCode).toBe(0) on line 32, which violates the CLAUDE.md convention that stdout/stderr assertions should precede the exitCode assertion for more useful error messages on failure. Consider moving the stdout assertion before the exitCode check:
| expect(stderr).not.toContain("Segmentation fault"); | |
| expect(stderr).not.toContain("Bun has crashed"); | |
| expect(exitCode).toBe(0); | |
| expect(stdout).toContain("SUCCESS"); | |
| expect(stderr).not.toContain("Segmentation fault"); | |
| expect(stderr).not.toContain("Bun has crashed"); | |
| expect(stdout).toContain("SUCCESS"); | |
| expect(exitCode).toBe(0); |
Why this is a problem
Issue
The test MessageChannel between workers does not crash with rapid messages checks expect(exitCode).toBe(0) on line 32 before checking expect(stdout).toContain("SUCCESS") on line 33. This violates the project's CLAUDE.md convention for assertion ordering when spawning processes in tests.
CLAUDE.md Rule Violated
The root CLAUDE.md states:
When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure.
Additionally, the test/CLAUDE.md reinforces this pattern through its example code (lines 48-50 and 93-95), which consistently show stdout and stderr assertions placed before the exitCode assertion.
Why This Still Matters
While the stderr assertions on lines 30-31 (expect(stderr).not.toContain("Segmentation fault") and expect(stderr).not.toContain("Bun has crashed")) already come before the exitCode check, the CLAUDE.md rule specifically calls out stdout assertions. The expect(stdout).toContain("SUCCESS") assertion on line 33 is the positive verification that the test actually produced the expected output. If a process exits with code 0 but fails to print "SUCCESS" (e.g., due to a deadlock that gets resolved by a timeout, or the worker completing without running the expected code path), the exitCode assertion would pass silently, and the developer would not immediately see the stdout mismatch as the root cause.
Suggested Fix
Reorder the assertions so that expect(stdout).toContain("SUCCESS") appears before expect(exitCode).toBe(0), matching the project convention:
expect(stderr).not.toContain("Segmentation fault");
expect(stderr).not.toContain("Bun has crashed");
expect(stdout).toContain("SUCCESS");
expect(exitCode).toBe(0);
|
Thanks for the thorough root-cause analysis here; the diagnosis (unsynchronized This was ultimately fixed by #29937 (shipped in bun 1.3.14), which went further and removed the Equivalent regression coverage lives in Closing as superseded. |
5 participants
Problem
MessageChannel would crash with a segmentation fault when workers exchanged many messages rapidly (e.g., using setInterval). The issue manifested in two ways:
Root Cause
Both MessagePortChannel and MessagePortChannelRegistry had critical thread safety issues. These classes were being accessed from multiple worker threads simultaneously without any synchronization:
MessagePortChannel - No locks protecting:
MessagePortChannelRegistry - Global singleton with no locks protecting:
When Worker1 and Worker2 both called postMessageToRemote() or takeAllMessagesForPort() concurrently, race conditions on Vector and HashMap operations led to memory corruption and crashes.
Solution
MessagePortChannel Thread Safety
MessagePortChannelRegistry Thread Safety
Deadlock Prevention
To avoid deadlocks (since both Registry and Channel have locks), the pattern used is:
This ensures we never hold two locks simultaneously while maintaining thread safety.
Files Modified
Fixes: #25805, #16186