Make helper-thread spawn failures non-fatal#311
Conversation
Add WTF::Thread::tryCreate (returns nullptr instead of RELEASE_ASSERT
when pthread_create/_beginthreadex is refused) and route the
process-survivable callers through it:
- AutomaticThread::start: on failure, stay in the no-underlying-thread
state so the next notify retries. Covers the GC marker pool
(ParallelHelperPool), the JIT/Wasm worklists, and the per-VM Heap
collector thread.
- Heap::finishRelinquishingConn: if the collector thread could not be
spawned, take the conn back so the mutator drives the collection
instead of parking forever in waitForCollector.
- pas_scavenger: on pthread_create failure, drop back to the
no-thread state and let the next eligibility notification retry
instead of PAS_ASSERTing.
- VMTraps: create the signal-sender WorkQueue eagerly in
initializeSignals so it is spawned while the process still has
thread budget rather than on the first fireTrap.
A container that caps pids.max (or a user at RLIMIT_NPROC) can hit
pthread_create EAGAIN on any of these when an embedder churns VMs; the
process now degrades (slower GC/JIT, fewer helpers) instead of
aborting.
|
Companion bun-side regression test: oven-sh/bun#34705 |
WalkthroughChangesThe pull request adds nullable thread creation handling in WTF and updates AutomaticThread, VMTraps, JavaScriptCore heap collection, and pas scavenging paths to handle thread creation failures without unconditional termination. Thread creation resilience
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/heap/Heap.cpp`:
- Around line 2262-2267: Update the thread-creation failure fallback in
releaseAccessSlow() to restore both mutatorHasConnBit and the stoppedBit set
during finishRelinquishingConn(), before the mutator can attempt to acquire
access. Ensure the resulting world state allows acquireAccessSlow() to proceed
rather than park indefinitely, while preserving the existing connection-reclaim
behavior.
In `@Source/JavaScriptCore/runtime/VMTraps.cpp`:
- Around line 369-374: Update the VM trap queue initialization around queue() so
Thread::create failures are handled without aborting. Make the trap-queue
creation API fallible and propagate an initialization failure, or provide a safe
fallback before VM setup completes; ensure later fireTrap() paths handle an
unavailable queue without terminating the process.
In `@Source/WTF/wtf/Threading.cpp`:
- Line 338: Correct the comment near Thread::entryPoint to state that the local
Ref context releases the remaining reference when the scope exits, rather than
attributing that cleanup to MutexLocker locker. Keep the comment focused on
undoing the speculative +1.
In `@Source/WTF/wtf/Threading.h`:
- Around line 141-142: Update the ownership comment for the thread-creation API
near NewThreadContext/tryCreate to state that entryPoint is consumed regardless
of success: on failure, the moved Function is destroyed and its captures
released, so the caller must not reuse or reclaim the closure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f27314f7-d275-48bc-9237-8d61cc238678
📒 Files selected for processing (6)
Source/JavaScriptCore/heap/Heap.cppSource/JavaScriptCore/runtime/VMTraps.cppSource/WTF/wtf/AutomaticThread.cppSource/WTF/wtf/Threading.cppSource/WTF/wtf/Threading.hSource/bmalloc/libpas/src/libpas/pas_scavenger.c
| if (!m_thread->hasUnderlyingThread(locker)) [[unlikely]] { | ||
| // The collector thread couldn't spawn (pthread_create EAGAIN at a | ||
| // pids/thread limit). Take the conn back so the mutator drives the | ||
| // collection; otherwise waitForCollector() would park forever. | ||
| m_worldState.exchangeOr(mutatorHasConnBit); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Clear the stopped state when reclaiming the connection.
releaseAccessSlow() sets stoppedBit at Lines [2218-2225] before calling finishRelinquishingConn(). On thread-creation failure, this code restores only mutatorHasConnBit; acquireAccessSlow() checks stoppedBit first and parks, leaving no collector thread available to clear it. The fallback can therefore deadlock the mutator. Restore the connection and undo the stop state established by that handoff before unparking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/heap/Heap.cpp` around lines 2262 - 2267, Update the
thread-creation failure fallback in releaseAccessSlow() to restore both
mutatorHasConnBit and the stoppedBit set during finishRelinquishingConn(),
before the mutator can attempt to acquire access. Ensure the resulting world
state allows acquireAccessSlow() to proceed rather than park indefinitely, while
preserving the existing connection-reclaim behavior.
| // Create the signal-sender WorkQueue (and its backing thread) now, | ||
| // while the process still has thread-budget headroom, instead of on | ||
| // the first fireTrap() which may arrive after workers have consumed | ||
| // every slot of a cgroup pids.max budget. RunLoop::create() cannot | ||
| // recover from a failed Thread::create. | ||
| queue(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'WorkQueue::create|RunLoop::create|Thread::create' Source/WTF Source/JavaScriptCoreRepository: oven-sh/WebKit
Length of output: 6531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== VMTraps.cpp excerpt ==\n'
sed -n '330,390p' Source/JavaScriptCore/runtime/VMTraps.cpp
printf '\n== WorkQueue.cpp excerpt ==\n'
sed -n '1,240p' Source/WTF/wtf/WorkQueue.cpp
printf '\n== generic WorkQueue implementation ==\n'
sed -n '1,140p' Source/WTF/wtf/generic/WorkQueueGeneric.cpp
printf '\n== RunLoop.cpp excerpt ==\n'
sed -n '1,170p' Source/WTF/wtf/RunLoop.cpp
printf '\n== Threading.cpp create signature area ==\n'
sed -n '300,380p' Source/WTF/wtf/Threading.cppRepository: oven-sh/WebKit
Length of output: 18641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [
Path("Source/JavaScriptCore/runtime/VMTraps.cpp"),
Path("Source/WTF/wtf/WorkQueue.cpp"),
Path("Source/WTF/wtf/generic/WorkQueueGeneric.cpp"),
Path("Source/WTF/wtf/RunLoop.cpp"),
]:
text = p.read_text()
print(f"\n== {p} contains create paths ==")
for i, line in enumerate(text.splitlines(), 1):
if "create(" in line or "Thread::create" in line or "RunLoop::create" in line or "WorkQueue::create" in line:
print(f"{i}: {line}")
PYRepository: oven-sh/WebKit
Length of output: 1197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== WorkQueue / RunLoop fallible APIs ==\n'
rg -n '\btryCreate\b|Failed to create thread|RELEASE_ASSERT_WITH_MESSAGE\(thread' Source/WTF Source/JavaScriptCore
printf '\n== VMTraps uses queue() or signal sender ==\n'
rg -n 'VMTraps::queue|SignalSender|fireTrap\(' Source/JavaScriptCore/runtime/VMTraps.cpp Source/JavaScriptCore/runtimeRepository: oven-sh/WebKit
Length of output: 48829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'initializeSignals\(' Source/JavaScriptCore
sed -n '660,760p' Source/JavaScriptCore/runtime/VM.cpp
sed -n '1,220p' Source/JavaScriptCore/runtime/VM.hRepository: oven-sh/WebKit
Length of output: 10152
Make VM trap queue creation fallible.
queue() still hard-aborts if Thread::create fails under pids.max/RLIMIT_NPROC exhaustion. Creating it eagerly here only moves that failure earlier; consider a fallible trap-queue API or a safe fallback before initialization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/runtime/VMTraps.cpp` around lines 369 - 374, Update the
VM trap queue initialization around queue() so Thread::create failures are
handled without aborting. Make the trap-queue creation API fallible and
propagate an initialization failure, or provide a safe fallback before VM setup
completes; ensure later fireTrap() paths handle an unavailable queue without
terminating the process.
| bool success = thread->establishHandle(context.get(), stackSpec, qos, schedulingPolicy); | ||
| RELEASE_ASSERT(success); | ||
| if (!thread->establishHandle(context.get(), stackSpec, qos, schedulingPolicy)) { | ||
| // Undo the speculative +1 for Thread::entryPoint; the locker's Ref drops the rest. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the refcount cleanup comment.
MutexLocker locker does not own a Ref; the local Ref context drops the remaining reference when the scope exits.
Suggested wording
- // Undo the speculative +1 for Thread::entryPoint; the locker's Ref drops the rest.
+ // Undo the speculative +1 for Thread::entryPoint; the local Ref drops the rest.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Undo the speculative +1 for Thread::entryPoint; the locker's Ref drops the rest. | |
| // Undo the speculative +1 for Thread::entryPoint; the local Ref drops the rest. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/WTF/wtf/Threading.cpp` at line 338, Correct the comment near
Thread::entryPoint to state that the local Ref context releases the remaining
reference when the scope exits, rather than attributing that cleanup to
MutexLocker locker. Keep the comment focused on undoing the speculative +1.
| // Returns nullptr if thread creation failed. The caller still owns entryPoint | ||
| // resources in that case (the moved Function is destroyed, its captures drop). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify ownership after failed thread creation.
entryPoint is moved into NewThreadContext; on failure, tryCreate destroys that Function and releases its captures. The caller cannot still own or reuse the moved closure, so document that it is consumed even on failure.
Suggested wording
- // Returns nullptr if thread creation failed. The caller still owns entryPoint
- // resources in that case (the moved Function is destroyed, its captures drop).
+ // Returns nullptr if thread creation failed. The entryPoint is consumed even
+ // on failure; the moved Function and its captures are destroyed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Returns nullptr if thread creation failed. The caller still owns entryPoint | |
| // resources in that case (the moved Function is destroyed, its captures drop). | |
| // Returns nullptr if thread creation failed. The entryPoint is consumed even | |
| // on failure; the moved Function and its captures are destroyed. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/WTF/wtf/Threading.h` around lines 141 - 142, Update the ownership
comment for the thread-creation API near NewThreadContext/tryCreate to state
that entryPoint is consumed regardless of success: on failure, the moved
Function is destroyed and its captures released, so the caller must not reuse or
reclaim the closure.
Preview Builds
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (!thread) { | ||
| // pthread_create/_beginthreadex refused (e.g. EAGAIN at a thread/pids | ||
| // limit). Stay in the no-underlying-thread state so the next notify | ||
| // retries instead of bringing the whole process down. | ||
| m_hasUnderlyingThread = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Swallowing the spawn failure here can hang JITWorklist::waitUntilAllPlansForVMAreReady forever. When all JITWorklistThreads have timed out (default 10 s idle) and start() fails at the pids limit, wakeThreads() still increments m_numberOfActiveThreads and leaves the plan at Preparing with no thread to advance it, so any later Heap::completeAllJITPlans() (via deleteAllCodeBlocks, forEachCodeBlockImpl, or the Debugger:: paths) parks indefinitely on m_planCompiledOrCancelled. The PR added a hasUnderlyingThread fallback for the collector in finishRelinquishingConn; JITWorklist::wakeThreads needs the same treatment (fall through to the synchronous !useConcurrentJIT compile-in-caller path, or drop the plan).
Extended reasoning...
What the bug is
The PR's premise for AutomaticThread::start() is that on tryCreate failure it can "stay in the no-underlying-thread state so the next notify retries" and JIT is merely "deferred". That is only safe if every caller that later waits for the AutomaticThread's work is prepared for the work never happening. JITWorklist is not: it has a blocking wait (waitUntilAllPlansForVMAreReady) that assumes a compiler thread will eventually move every enqueued plan to Ready.
Code path
JITWorklistThread is constructed with the default AutomaticThread timeout (JITWorklistThread.cpp:81 passes only (locker, lock, cond, ThreadType::Compiler) — no timeout override, so 10_s from AutomaticThread.h). After 10 s idle the underlying OS thread exits via stopForTimeout, leaving m_hasUnderlyingThread == false. This is a normal steady state in any long-running process with a quiet period; it also holds for the very first JIT compilation.
When a plan is enqueued in that state, JITWorklist::enqueue() adds it to m_plans / m_queues[tier] at stage Preparing and calls wakeThreads() (JITWorklist.cpp:175-179). wakeThreads (JITWorklist.cpp:144-147) does:
while (m_numberOfActiveThreads < targetNumThreads) {
m_planEnqueued->notifyOne(locker);
m_numberOfActiveThreads++;
}AutomaticThreadCondition::notifyOne finds a thread with !hasUnderlyingThread and calls start(). With this PR, start() sets m_hasUnderlyingThread = false and returns silently when Thread::tryCreate fails. wakeThreads still increments m_numberOfActiveThreads. That counter is only decremented in JITWorklistThread::poll() (JITWorklistThread.cpp:122-123), which requires a running OS thread — so the count is now permanently inflated, and later enqueues may skip the notify entirely because m_numberOfActiveThreads >= targetNumThreads.
Why nothing rescues it
The plan sits in m_plans at stage Preparing with nobody to advance it. waitUntilAllPlansForVMAreReady (JITWorklist.cpp:286-301) loops while any plan for this VM has stage != Ready and blocks on m_planCompiledOrCancelled.wait(*m_lock). That condition is notified only by a compiler thread's work() completion (JITWorklistThread.cpp:167) or by removeDeadPlans (JITWorklist.cpp:478). Neither fires: there is no compiler thread, and even if a collector-driven removeDeadPlans woke the waiter, the plan is still Preparing for a live VM so the loop re-waits. cancelAllPlansForVM (VM shutdown) is safe because it removes stage-!=Compiling plans first; completeAllPlansForVM does not.
completeAllJITPlans is reachable via Heap::forEachCodeBlockImpl (Heap.cpp:2837), Heap::deleteAllCodeBlocks (Heap.cpp:1141), and four Debugger:: paths (setSteppingMode, toggleBreakpoint, clearBreakpoints, clearDebuggerRequests).
Impact
Before this PR the process would RELEASE_ASSERT inside Thread::create at step start() and be restarted by its supervisor. After this PR the same resource-exhaustion scenario the PR targets produces an indefinite hang holding all live requests — the exact outcome the PR is trying to avoid, but harder for a supervisor to detect than a SIGABRT. The PR description's "deferred JIT" claim does not hold once anything waits for JIT completion.
Step-by-step proof
- Bun process idles ≥10 s → all JIT worklist OS threads exit via
stopForTimeout; everyJITWorklistThreadhasm_hasUnderlyingThread == false. - Workers churn until the process reaches
pids.max/RLIMIT_NPROC. - A function tiers up:
JITWorklist::enqueue(plan)→m_plans.add(key, plan)(stagePreparing) →wakeThreads(). wakeThreads→notifyOne→AutomaticThread::start→Thread::tryCreatereturnsnullptr→ new branch setsm_hasUnderlyingThread = false, returns. Back inwakeThreads,m_numberOfActiveThreads++.- No thread ever runs
poll()/work(); the plan staysPreparing. - Something calls
Heap::completeAllJITPlans()(e.g.Heap::deleteAllCodeBlockson worker teardown, or aDebuggerattach). completeAllPlansForVM→waitUntilAllPlansForVMAreReadyfinds thePreparingplan →m_planCompiledOrCancelled.wait(*m_lock)forever.
Fix
Mirror the collector-thread fallback the PR already added in finishRelinquishingConn: after notifyOne in JITWorklist::wakeThreads, check whether a thread with an underlying OS thread actually exists; if none does, either fall through to the existing synchronous compile path (the !Options::useConcurrentJIT() branch at the top of enqueue, JITWorklist.cpp:153-163) or drop the plan and return CompilationDeferred without adding it to m_plans. Alternatively, have AutomaticThread::start / notifyOne report failure so callers can react.
| if (!m_thread->hasUnderlyingThread(locker)) [[unlikely]] { | ||
| // The collector thread couldn't spawn (pthread_create EAGAIN at a | ||
| // pids/thread limit). Take the conn back so the mutator drives the | ||
| // collection; otherwise waitForCollector() would park forever. | ||
| m_worldState.exchangeOr(mutatorHasConnBit); | ||
| } |
There was a problem hiding this comment.
🔴 Re-setting mutatorHasConnBit here breaks both callers when the collector fails to spawn. The no-arg relinquishConn() at line 2274 is while (relinquishConn(m_worldState.load())) { } — it CAS-clears the bit, calls finishRelinquishingConn() which re-sets it, returns true, and loops forever burning pthread_create syscalls (100 % CPU spin at exactly the pids limit this PR targets). And when reached from releaseAccessSlow() (line 2216-2229), stoppedBit has already been set and hasAccessBit cleared, so re-setting the conn produces stoppedBit | mutatorHasConnBit with no collector — acquireAccessSlow() then parks forever on stoppedBit. The fallback needs to signal spawn failure without re-setting the bit inside the loop (e.g. return a status from finishRelinquishingConn() and break out of relinquishConn(), and clear/skip stoppedBit on the release-access path).
Extended reasoning...
What the bug is
The new fallback in finishRelinquishingConn() does m_worldState.exchangeOr(mutatorHasConnBit) when the collector AutomaticThread has no underlying thread after notifyOne(). This function has two callers, and the fallback is incorrect for both.
Path 1: infinite spin in relinquishConn()
Heap::relinquishConn() (no-arg) at Heap.cpp:2272-2274 is:
while (relinquishConn(m_worldState.load())) { }relinquishConn(oldState) (2235-2251) returns false only when !(oldState & mutatorHasConnBit) (line 2240) or m_threadShouldStop (line 2243). On a successful CAS it clears mutatorHasConnBit, calls finishRelinquishingConn(), and returns true — the loop is designed to terminate on the next iteration when the reload sees the bit clear.
Step-by-step under a thread/pids limit:
- Iteration 1 loads
oldStatewithmutatorHasConnBitset. - CAS at line 2246 clears the bit;
finishRelinquishingConn()runs. m_requestsis non-empty (only the un-spawnable collector drains it), sonotifyOne()→AutomaticThread::start()→Thread::tryCreate()→pthread_createreturnsEAGAIN→m_hasUnderlyingThread = false.!m_thread->hasUnderlyingThread(locker)is true →exchangeOr(mutatorHasConnBit)re-sets the bit.relinquishConn(oldState)returnstrue.- Iteration 2 reloads
m_worldState, seesmutatorHasConnBitset again → back to step 2. Forever.
m_threadShouldStop stays false (it's only set at heap teardown), so the loop never exits. This is called from waitForCollector() at line 2161 — exactly the path the new comment says it's protecting. A synchronous GC (collectSync() → waitForCollection() → waitForCollector()) in a process at its RLIMIT_NPROC/pids.max budget — the PR's motivating repro — now busy-spins on the mutator issuing pthread_create in a tight loop instead of driving the collection itself. This trades the old SIGABRT for a 100 % CPU hang, which defeats the PR's stated goal of graceful degradation.
Path 2: stoppedBit hang via releaseAccessSlow()
The second caller is releaseAccessSlow() (Heap.cpp:2216-2229). When (oldState & mutatorHasConnBit) && m_nextPhase != m_currentPhase, it CASes to a state that clears hasAccessBit | mutatorHasConnBit and sets stoppedBit (line 2224, with the comment "the collector will be awoken"), then calls finishRelinquishingConn(). If spawn fails, the fallback ORs mutatorHasConnBit back in, producing stoppedBit | mutatorHasConnBit with no hasAccessBit and no collector. acquireAccessSlow() (line 2179-2186) then parks forever on stoppedBit — nothing will ever clear it (only the collector's resumeTheMutator() does), and the mutator can't drive the collection because it has released access. This state also violates the invariant asserted at stopTheMutator() line 2006 (RELEASE_ASSERT(!(oldState & mutatorHasConnBit)) under stoppedBit), so if a collector does eventually spawn it will crash there.
This second path is narrower — it needs m_nextPhase != m_currentPhase with the mutator holding the conn, which arises when the collector's finishChangingPhase(Collector) handed off via stopTheMutator() and the underlying HeapThread then timed out (10 s idle) before the mutator released access. But it's the same class of hang the PR is trying to eliminate, and the fix touches the same lines.
Why existing code doesn't prevent it
Before this PR, AutomaticThread::start would RELEASE_ASSERT on spawn failure, so finishRelinquishingConn() never returned with !hasUnderlyingThread. The while-loop in relinquishConn() and the stoppedBit handshake in releaseAccessSlow() were both written under the assumption that notifyOne() guarantees a running collector.
Suggested fix
Have finishRelinquishingConn() return whether the collector actually has an underlying thread, and let each caller react: relinquishConn() breaks out of the while-loop with the conn kept (so waitForCollector()'s next stopIfNecessarySlow() drives collectInMutatorThread()), and releaseAccessSlow() either avoids setting stoppedBit when !hasUnderlyingThread or clears it in the fallback. Simply doing exchangeOr(mutatorHasConnBit) inside the shared helper cannot satisfy both call sites.
| if (!thread->establishHandle(context.get(), stackSpec, qos, schedulingPolicy)) { | ||
| // Undo the speculative +1 for Thread::entryPoint; the locker's Ref drops the rest. | ||
| context->deref(); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
🔴 On Windows, when _beginthreadex fails and tryCreate returns nullptr, the local Ref<Thread> destructs and Thread::~Thread() runs if (m_handle != INVALID_HANDLE_VALUE) CloseHandle(m_handle) — but m_handle has no default initializer and was never assigned (assignment happens in establishPlatformSpecificHandle, which is skipped on failure). Previously the RELEASE_ASSERT(success) aborted before ~Thread() could run; now the recovery path reads an indeterminate handle and can CloseHandle() an unrelated live handle. Give m_handle a default initializer (e.g. INVALID_HANDLE_VALUE on Windows) in Threading.h.
Extended reasoning...
What the bug is
Thread::m_handle is declared in Source/WTF/wtf/Threading.h as PlatformThreadHandle m_handle; with no default initializer, and the Thread(SchedulingPolicy, IsMain) constructor only initializes m_isRealtime and m_uid. On Windows, Thread::~Thread() (ThreadingWin.cpp:102-108) does:
if (m_handle != INVALID_HANDLE_VALUE)
CloseHandle(m_handle);When establishHandle() fails, m_handle has never been written, so the destructor reads an indeterminate value and — since INVALID_HANDLE_VALUE is (HANDLE)-1 — almost certainly passes the check and calls CloseHandle() on garbage.
Code path that triggers it
In the new Thread::tryCreate (Threading.cpp:337-341):
if (!thread->establishHandle(context.get(), stackSpec, qos, schedulingPolicy)) {
context->deref();
return nullptr;
}On Windows, establishHandle (ThreadingWin.cpp:153-168) returns false at line 164 when _beginthreadex fails — before line 166's establishPlatformSpecificHandle(threadHandle, threadIdentifier), which is the only place m_handle is assigned for a newly created thread.
Step-by-step proof
AutomaticThread::start→Thread::tryCreate(...).adoptRef(*new Thread(schedulingPolicy, IsMain::No))constructs aThread; the ctor sets onlym_isRealtimeandm_uid.m_handleis left indeterminate.NewThreadContextis created holding a secondRef<Thread>(refcount = 2), plus a speculativecontext->ref()for the entry point._beginthreadexreturns 0 (thread limit hit) →establishHandlereturnsfalseat line 164;establishPlatformSpecificHandlenever runs.tryCreatedoescontext->deref()(undo speculative ref) andreturn nullptr.- Unwinding:
Ref contextdestructs →NewThreadContextdestructs → itsRef<Thread>drops (2→1). ThenRef threaddestructs (1→0) →Thread::~Thread()runs. ~Thread()reads the never-initializedm_handle, compares against(HANDLE)-1, and callsCloseHandle(garbage).
Why existing code doesn't prevent it
Before this PR, step 5 was RELEASE_ASSERT(success) — the process aborted before the local Ref<Thread> could destruct, so ~Thread() never ran with an uninitialized m_handle. This PR removes that abort specifically to let the process survive, which for the first time makes ~Thread() reachable on a Thread whose handle was never established. POSIX is unaffected because Thread::~Thread() = default there (ThreadingPOSIX.cpp:83).
Impact
This is UB on the exact scenario the PR is designed to survive on Windows (_beginthreadex refusal at a thread limit — explicitly called out in the PR description). CloseHandle() on an arbitrary bit pattern can silently close an unrelated live handle (file, event, thread, socket) elsewhere in the process, turning graceful degradation into non-deterministic corruption. Even if the allocator happened to zero the memory, NULL != INVALID_HANDLE_VALUE, so CloseHandle(NULL) would still be called.
Fix
Give m_handle a default initializer in Threading.h, e.g.:
#if OS(WINDOWS)
PlatformThreadHandle m_handle { INVALID_HANDLE_VALUE };
#else
PlatformThreadHandle m_handle { };
#endifor set m_handle = INVALID_HANDLE_VALUE in the Windows establishHandle failure branch before returning false.
1 participant
Add
WTF::Thread::tryCreate(returnsnullptrinstead ofRELEASE_ASSERTwhenpthread_create/_beginthreadexis refused) and route the process-survivable callers through it.Motivation
A Bun app that churns
Workers inside a container with apids.maxcgroup budget (or a user atRLIMIT_NPROC) can hitpthread_createEAGAIN on any of JSC's lazily-spawned helper threads. Every one of those paths currentlyRELEASE_ASSERTs orPAS_ASSERTs, so the whole process is killed with all its live requests instead of just that oneWorkerfailing. Node at the same limit fails the Worker and recovers.Repro (as any non-root user on Linux):
bash -c 'ulimit -u 16 && exec bun worker-churn.mjs'with
worker-churn.mjscreating and terminating a fewWorkers in a loop:SIGABRTinWTF::Thread::create/pas_scavenger_notify_eligibility_if_needed.Changes
WTF::Thread::tryCreate: new fallible variant.Thread::createnow delegates to it and keeps theRELEASE_ASSERT(with the thread name in the message) for callers that cannot tolerate failure.AutomaticThread::start: usetryCreate; on failure stay in the no-underlying-thread state so the nextnotifyOne/notifyAllretries. Covers the GC markerParallelHelperPool, the JIT/Wasm worklists, and the per-VMHeap::HeapThread.Heap::finishRelinquishingConn: if the collector thread has no underlying thread afternotifyOne, re-setmutatorHasConnBitso the mutator drives the collection instead of parking forever inwaitForCollector.pas_scavenger_notify_eligibility_if_needed: onpthread_createfailure, drop back topas_scavenger_state_no_threadinstead ofPAS_ASSERTing; the nextdid_create_eligibleretries.VMTraps::initializeSignals: create the signal-senderWorkQueueeagerly so its backing thread is spawned while there is still thread budget rather than on the firstfireTrap(which arrives after Workers have consumed every slot;RunLoop::createcannot recover).With these, a process at a thread budget degrades (slower GC, deferred JIT, fewer marking helpers) instead of aborting. Verified on the bun side across
RLIMIT_NPROC13..28: every value now exits cleanly where it previouslySIGABRTed.Companion regression test lives in oven-sh/bun; this PR is the load-bearing half.