Skip to content
This repository was archived by the owner on Jan 31, 2020. It is now read-only.

Commit 21375e8

Browse files
committed
Fix issue with iterative removal
In testing zend-eventmanager, I discovered a race condition with the following: ```php while ($queue->contains($listener)) { $queue->remove($listener); } ``` In debugging, I determined that while `remove()` was correctly identifying that it had the datum, it was not using the correct index in the value array to unset it, which would leave it in place. Modifying the code to use `key()` on the values array allows it to work as expected.
1 parent c5de3c2 commit 21375e8

File tree

2 files changed

+49
-1
lines changed

2 files changed

+49
-1
lines changed

src/FastPriorityQueue.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,8 @@ public function remove($datum)
135135
$this->rewind();
136136
while ($this->valid()) {
137137
if (current($this->values[$this->maxPriority]) === $datum) {
138-
unset($this->values[$this->maxPriority][$this->subIndex]);
138+
$index = key($this->values[$this->maxPriority]);
139+
unset($this->values[$this->maxPriority][$index]);
139140
--$this->count;
140141
return true;
141142
}

test/FastPriorityQueueTest.php

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,4 +238,51 @@ public function testRewindShouldNotRaiseErrorWhenQueueIsEmpty()
238238
$queue->rewind();
239239
restore_error_handler();
240240
}
241+
242+
public function testRemoveShouldFindItemEvenIfMultipleItemsAreInQueue()
243+
{
244+
$prototype = function ($e) {
245+
};
246+
247+
$queue = new FastPriorityQueue();
248+
$this->assertTrue($queue->isEmpty());
249+
250+
$listeners = [];
251+
for ($i = 0; $i < 5; $i += 1) {
252+
$listeners[] = $listener = clone $prototype;
253+
$queue->insert($listener, 1);
254+
}
255+
256+
$remove = array_rand(array_keys($listeners));
257+
$listener = $listeners[$remove];
258+
259+
$this->assertTrue($queue->contains($listener));
260+
$this->assertTrue($queue->remove($listener));
261+
$this->assertFalse($queue->contains($listener));
262+
}
263+
264+
public function testIterativelyRemovingItemsShouldRemoveAllItems()
265+
{
266+
$prototype = function ($e) {
267+
};
268+
269+
$queue = new FastPriorityQueue();
270+
$this->assertTrue($queue->isEmpty());
271+
272+
$listeners = [];
273+
for ($i = 0; $i < 5; $i += 1) {
274+
$listeners[] = $listener = clone $prototype;
275+
$queue->insert($listener, 1);
276+
}
277+
278+
for ($i = 0; $i < 5; $i += 1) {
279+
$listener = $listeners[$i];
280+
$queue->remove($listener);
281+
}
282+
283+
for ($i = 0; $i < 5; $i += 1) {
284+
$listener = $listeners[$i];
285+
$this->assertFalse($queue->contains($listener), sprintf('Listener %s remained in queue', $i));
286+
}
287+
}
241288
}

0 commit comments

Comments
 (0)