Skip to content

Commit b87600a

Browse files
committed
fix(allocator): fix potential deadlock in FixedSizeAllocatorPool (#17112)
#17023 introduced a queuing system to limit the number of `FixedSizeAllocator`s in play at any given time. However, there was a subtle race condition, which could result in deadlock. Fix it. See comments in the code for explanation.
1 parent 938f23b commit b87600a

File tree

1 file changed

+16
-12
lines changed

1 file changed

+16
-12
lines changed

crates/oxc_allocator/src/pool/fixed_size.rs

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ impl FixedSizeAllocatorPool {
7575
// Try to get an allocator from the pool.
7676
// This is in a block, so that `Mutex` lock is held for the shortest possible time.
7777
let maybe_allocator = {
78-
let mut allocators = self.allocators.lock().unwrap();
79-
allocators.pop()
78+
let mut allocators_guard = self.allocators.lock().unwrap();
79+
allocators_guard.pop()
8080
};
8181
if let Some(allocator) = maybe_allocator {
8282
return allocator.into_inner();
@@ -87,16 +87,18 @@ impl FixedSizeAllocatorPool {
8787
return allocator.into_inner();
8888
}
8989

90-
// Pool cannot produce another allocator. Wait for an existing allocator to be returned to the pool.
90+
// Pool cannot produce another allocator.
91+
// Check if an allocator was returned to the pool by another thread during the time when we were trying
92+
// to create a new allocator above. If not, wait for notification that the pool isn't empty any more.
93+
// IMPORTANT: To avoid deadlock, we must check if pool is still empty BEFORE waiting.
94+
// Otherwise, another thread could have added an allocator to the pool since we last checked
95+
// at start of the function, and `self.available.wait(allocators_guard)` could wait forever.
96+
let mut allocators_guard = self.allocators.lock().unwrap();
9197
loop {
92-
// This is in a block, so that `Mutex` lock is held for the shortest possible time
93-
let maybe_allocator = {
94-
let mut allocators = self.available.wait(self.allocators.lock().unwrap()).unwrap();
95-
allocators.pop()
96-
};
97-
if let Some(allocator) = maybe_allocator {
98+
if let Some(allocator) = allocators_guard.pop() {
9899
return allocator.into_inner();
99100
}
101+
allocators_guard = self.available.wait(allocators_guard).unwrap();
100102
}
101103
}
102104

@@ -142,10 +144,12 @@ impl FixedSizeAllocatorPool {
142144
FixedSizeAllocator { allocator: ManuallyDrop::new(allocator) };
143145
fixed_size_allocator.reset();
144146

145-
// This is in a block, so that `Mutex` lock is held for the shortest possible time
147+
// This is in a block so that lock (`allocators_guard`) is released before notifying the `Condvar`.
148+
// This avoids waking up a thread, only for it to be immediately blocked again because the `Mutex`
149+
// is still locked.
146150
{
147-
let mut allocators = self.allocators.lock().unwrap();
148-
allocators.push(fixed_size_allocator);
151+
let mut allocators_guard = self.allocators.lock().unwrap();
152+
allocators_guard.push(fixed_size_allocator);
149153
}
150154

151155
self.available.notify_one();

0 commit comments

Comments
 (0)