Refactoring pools for concurrent operations #24
Conversation
- Removed existing tests for Group and Pool classes. - Introduced a new Adapter system for managing pools with Stack and Swoole implementations. - Created a custom Exception class for better error handling in pool operations. - Added telemetry support for monitoring pool usage. - Implemented connection management with reclaim and destroy functionalities. - Established a base test class for consistent testing across different adapter implementations. - Enhanced test coverage for connection and pool functionalities.
|
Warning Rate limit exceeded@ArnabChatterjee20k has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 17 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an abstract Adapter API and two adapters: Stack (in-memory array) and Swoole (Channel + Lock). Refactors Pool to depend on an adapter with lazy connection creation, connection counting, synchronization timeout, and new create/destroy helpers; pool telemetry and availability semantics now use the adapter. Introduces test traits (ConnectionTestScope, PoolTestScope, GroupTestScope), a Base test class, and adapter-specific tests (StackTest, SwooleTest); removes legacy monolithic tests. composer.json updates dev dependencies and suggests ext-swoole. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 7
🧹 Nitpick comments (3)
src/Pools/Adapter.php (1)
7-7: Consider removing the unused$valueparameter fromfill().The
$valueparameter is not used by either concrete adapter implementation (Stack or Swoole). Both adapters perform lazy initialization and don't pre-fill the pool with values.If there's no plan to support pre-filling, consider simplifying the signature:
- abstract public function fill(int $size, mixed $value): static; + abstract public function fill(int $size): static;src/Pools/Adapter/Swoole.php (1)
20-21: Update terminology: "coroutine-safe" not "thread-safe".Swoole uses coroutines (cooperative multitasking), not threads. The comment should reflect this.
- // Initialize lock for thread-safe operations + // Initialize lock for coroutine-safe operationstests/Pools/Scopes/ConnectionTestScope.php (1)
79-97: Consider usingassertNotNullinstead of manual null check.The manual null check with exception is verbose. PHPUnit's
assertNotNullprovides the same guarantee with less code.- $pool = $this->connectionObject->getPool(); - - if ($pool === null) { - throw new Exception("Pool should never be null here."); - } - - $this->assertInstanceOf(Pool::class, $pool); + $pool = $this->connectionObject->getPool(); + $this->assertNotNull($pool); + $this->assertInstanceOf(Pool::class, $pool);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
composer.json(1 hunks)src/Pools/Adapter.php(1 hunks)src/Pools/Adapter/Stack.php(1 hunks)src/Pools/Adapter/Swoole.php(1 hunks)src/Pools/Pool.php(9 hunks)tests/Pools/Adapter/StackTest.php(1 hunks)tests/Pools/Adapter/SwooleTest.php(1 hunks)tests/Pools/Base.php(1 hunks)tests/Pools/ConnectionTest.php(0 hunks)tests/Pools/GroupTest.php(0 hunks)tests/Pools/PoolTest.php(0 hunks)tests/Pools/Scopes/ConnectionTestScope.php(1 hunks)tests/Pools/Scopes/GroupTestScope.php(1 hunks)tests/Pools/Scopes/PoolTestScope.php(1 hunks)
💤 Files with no reviewable changes (3)
- tests/Pools/PoolTest.php
- tests/Pools/ConnectionTest.php
- tests/Pools/GroupTest.php
🧰 Additional context used
🧬 Code graph analysis (6)
src/Pools/Adapter.php (3)
src/Pools/Adapter/Stack.php (4)
fill(12-17)push(19-24)pop(26-29)count(31-34)src/Pools/Adapter/Swoole.php (4)
fill(15-24)push(26-31)pop(33-39)count(42-45)src/Pools/Pool.php (3)
push(308-319)pop(225-262)count(326-330)
src/Pools/Adapter/Stack.php (3)
src/Pools/Adapter.php (5)
Adapter(5-18)fill(7-7)push(9-9)pop(15-15)count(17-17)src/Pools/Adapter/Swoole.php (4)
fill(15-24)push(26-31)pop(33-39)count(42-45)src/Pools/Pool.php (3)
push(308-319)pop(225-262)count(326-330)
tests/Pools/Base.php (6)
src/Pools/Adapter.php (1)
Adapter(5-18)tests/Pools/Adapter/SwooleTest.php (2)
getAdapter(13-16)execute(18-36)tests/Pools/Adapter/StackTest.php (2)
getAdapter(10-13)execute(15-18)tests/Pools/Scopes/ConnectionTestScope.php (2)
getAdapter(11-11)execute(12-12)tests/Pools/Scopes/GroupTestScope.php (2)
getAdapter(11-11)execute(12-12)tests/Pools/Scopes/PoolTestScope.php (2)
getAdapter(12-12)execute(13-13)
src/Pools/Adapter/Swoole.php (3)
src/Pools/Adapter.php (5)
Adapter(5-18)fill(7-7)push(9-9)pop(15-15)count(17-17)src/Pools/Adapter/Stack.php (4)
fill(12-17)push(19-24)pop(26-29)count(31-34)src/Pools/Pool.php (3)
push(308-319)pop(225-262)count(326-330)
tests/Pools/Scopes/GroupTestScope.php (1)
src/Pools/Group.php (4)
Group(8-129)add(19-23)get(30-33)remove(39-43)
src/Pools/Pool.php (3)
src/Pools/Adapter.php (5)
Adapter(5-18)fill(7-7)count(17-17)pop(15-15)push(9-9)src/Pools/Adapter/Stack.php (4)
fill(12-17)count(31-34)pop(26-29)push(19-24)src/Pools/Adapter/Swoole.php (4)
fill(15-24)count(42-45)pop(33-39)push(26-31)
🪛 PHPMD (2.15.0)
src/Pools/Adapter/Stack.php
12-12: Avoid unused parameters such as '$size'. (undefined)
(UnusedFormalParameter)
12-12: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
26-26: Avoid unused parameters such as '$timeout'. (undefined)
(UnusedFormalParameter)
tests/Pools/Adapter/SwooleTest.php
147-147: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
src/Pools/Adapter/Swoole.php
15-15: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (20)
tests/Pools/Adapter/StackTest.php (1)
8-19: LGTM!The test class properly implements the abstract base methods for the Stack adapter. The synchronous execution in
execute()is appropriate for the non-coroutine Stack adapter.src/Pools/Adapter/Stack.php (1)
12-29: Unused parameters are acceptable for this simple adapter.The static analysis warnings about unused parameters (
$size,$value,$timeout) are expected here. The Stack adapter is a simple in-memory implementation that doesn't require these features, but must implement the abstract Adapter interface.Note: If the unused parameters cause maintenance concerns, consider documenting why they're ignored:
public function fill(int $size, mixed $value): static { // Stack adapter doesn't pre-allocate; $size and $value are unused $this->pool = []; return $this; }tests/Pools/Base.php (1)
11-24: LGTM!The abstract base class provides an excellent test architecture using traits to compose test scopes. The
getAdapter()andexecute()abstractions allow concrete test classes to provide adapter-specific implementations while reusing common test logic.tests/Pools/Scopes/GroupTestScope.php (1)
9-140: LGTM!The test scope provides comprehensive coverage of Group functionality, including successful operations, error conditions, and the recursive
use()method. The tests properly utilize the abstractgetAdapter()andexecute()methods to support multiple adapter implementations.tests/Pools/Adapter/SwooleTest.php (6)
13-36: LGTM!The adapter implementation correctly returns a Swoole instance, and the
execute()method properly wraps callbacks in a Swoole coroutine context with exception propagation. This ensures all tests run in the appropriate coroutine environment.
38-116: Excellent concurrency test coverage.This test validates the core pooling behavior under concurrent access, including:
- Proper connection creation limits (exactly pool size)
- Connection reuse after reclaim
- Retry logic when pool is exhausted
The test correctly uses Swoole channels for synchronization and validates both success count and connection counter.
118-176: LGTM!This high concurrency test validates that 20 coroutines can share a pool of 3 connections, demonstrating proper queuing and retry behavior. The validation of exactly 3 connections created confirms no race conditions in connection initialization.
178-240: LGTM!This test effectively validates that concurrent
pop()operations don't return duplicate connections, which would indicate a race condition. The assertion of exactly 5 unique resources confirms thread-safety.
242-297: LGTM!This test validates connection reuse by confirming that the same connection IDs are returned in the second wave after reclaim. This ensures the pool properly recycles connections rather than creating new ones.
299-353: LGTM!The stress test with 100 concurrent requests sharing 10 connections validates the pool's behavior under heavy load. The variable sleep duration simulates realistic workload patterns.
Note: The unused
$resourceparameter on line 328 (flagged by static analysis) is intentional - the test just needs to exercise the pool mechanism.tests/Pools/Scopes/PoolTestScope.php (3)
10-23: Well-structured test trait design.The trait properly abstracts the adapter and execution context, enabling adapter-specific test implementations. The
setUpPool()helper provides consistent pool initialization across tests.
321-357: Good telemetry test coverage.The helper function
allocatecleanly manages connection lifecycle for telemetry assertions. The test validates open, active, and idle connection gauge values across multiple allocation cycles.
288-319: Good test for connection destruction and recreation.The test properly verifies that
destroy()creates new connections by using an incrementing counter in the init callback to distinguish old ('x') from new ('y') connections.tests/Pools/Scopes/ConnectionTestScope.php (2)
9-22: Clean trait structure matching PoolTestScope pattern.The trait follows the same abstract method pattern for adapter and execution context, providing consistent test scaffolding across the test suite.
99-165: Comprehensive connection lifecycle tests.The tests properly cover connection reclaim, exception handling for orphaned connections, and the destroy/recreate flow. The incrementing counter pattern in
testConnectionDestroycorrectly verifies new connection creation.src/Pools/Pool.php (5)
225-262: Lazy connection creation logic is correct.The
pop()method properly implements lazy initialization: it creates connections on-demand when the pool is empty and capacity remains. The retry logic with sleep is correctly structured.
270-302: Connection creation with retry logic looks correct.The
connectionsCreatedcounter is properly incremented/decremented on success/failure. The ID check at line 295 usesempty()which will catch bothnulland empty string.
390-402:isEmptyandisFullsemantics updated for lazy initialization.With lazy initialization:
isEmpty(): Returns true when the idle pool is empty (ignoring potential uncreated connections)isFull(): Returns true when no connections are active (all returned/available)These semantics align with the test expectations. The
count()method at line 329 captures "available capacity" differently.
70-77: Constructor properly initializes adapter-backed pool.The adapter is correctly initialized with
fill()for lazy initialization, and telemetry is set to a no-op adapter by default.
321-330:count()returns available capacity.The count semantics now reflect "available connections" = idle connections in pool + uncreated connection slots. This aligns with the lazy initialization model.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Pools/Pool.php (1)
383-392: Same race condition in batch destroy loop.The same synchronization issue exists here: connection creation (lines 389-391) occurs outside the lock after it's released at line 386.
Apply the same fix as the single-connection case to ensure the check and creation happen atomically within the lock.
🧹 Nitpick comments (2)
src/Pools/Adapter/Swoole.php (1)
37-37: Fix typo in comment."occured" should be "occurred".
- // if pool is empty or timeout occured => result will be false + // if pool is empty or timeout occurred => result will be falsesrc/Pools/Pool.php (1)
427-427: Telemetry metricstelemetryPoolCapacityandtelemetryOpenConnectionsrecord identical values.Line 427 records
$activeConnections + $this->pool->count(), which is identical to$openConnectionscomputed at line 422. This makes the two metrics redundant. Based on the previous discussion, you've clarified that "pool capacity" should track current pool connections, but this creates duplicate telemetry data.Consider either:
- Recording
$this->sizefortelemetryPoolCapacityto track the maximum configured capacity (typical observability pattern), or- Removing one of the duplicate metrics if they're intended to track the same value.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Pools/Adapter.php(1 hunks)src/Pools/Adapter/Stack.php(1 hunks)src/Pools/Adapter/Swoole.php(1 hunks)src/Pools/Pool.php(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Pools/Adapter.php (2)
src/Pools/Adapter/Stack.php (5)
fill(12-17)push(19-24)pop(26-29)count(31-34)withLock(36-39)src/Pools/Adapter/Swoole.php (5)
fill(15-24)push(26-31)pop(33-39)count(42-45)withLock(47-61)
src/Pools/Adapter/Swoole.php (3)
src/Pools/Adapter.php (6)
Adapter(5-27)fill(7-7)push(9-9)pop(15-15)count(17-17)withLock(26-26)src/Pools/Adapter/Stack.php (5)
fill(12-17)push(19-24)pop(26-29)count(31-34)withLock(36-39)src/Pools/Pool.php (3)
push(313-324)pop(225-267)count(331-335)
src/Pools/Adapter/Stack.php (3)
src/Pools/Adapter.php (6)
Adapter(5-27)fill(7-7)push(9-9)pop(15-15)count(17-17)withLock(26-26)src/Pools/Adapter/Swoole.php (5)
fill(15-24)push(26-31)pop(33-39)count(42-45)withLock(47-61)src/Pools/Pool.php (3)
push(313-324)pop(225-267)count(331-335)
🪛 PHPMD (2.15.0)
src/Pools/Adapter/Swoole.php
15-15: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
src/Pools/Adapter/Stack.php
12-12: Avoid unused parameters such as '$size'. (undefined)
(UnusedFormalParameter)
12-12: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
26-26: Avoid unused parameters such as '$timeout'. (undefined)
(UnusedFormalParameter)
36-36: Avoid unused parameters such as '$timeout'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (4)
src/Pools/Adapter/Swoole.php (2)
13-21: The Lock property is correctly used inwithLock()method.The previous review comment suggesting the Lock is unused appears to be incorrect. The
$lockproperty is properly utilized in thewithLock()method (lines 47-61) to provide mutual exclusion when executing callbacks that need synchronized access to shared state.
15-15: Unused$valueparameter is intentional for lazy initialization.The
$valueparameter is part of the Adapter interface signature but intentionally unused in this implementation, which follows a lazy initialization pattern where connections are created on-demand rather than pre-filled. This is consistent with the Stack adapter implementation and aligns with the PR's objective of lazy connection creation.src/Pools/Pool.php (1)
235-243: Verify that connection creation under lock doesn't cause deadlock.The
withLock()call wraps a check but thencreateConnection()is called outside the lock (line 240), which is correct. However, ensure that ifcreateConnection()internally needs the pool adapter for any operations, it won't attempt to acquire the same lock again (which would deadlock).Review the
createConnection()implementation (lines 275-307) to confirm it doesn't call any adapter methods that might acquire the lock.src/Pools/Adapter/Stack.php (1)
12-39: Simple synchronous adapter correctly implements the interface.The Stack adapter intentionally ignores several parameters (
$size,$value,$timeout) because it's designed as a simple, synchronous, in-memory implementation suitable for testing and non-concurrent scenarios. The parameter signatures must match the abstractAdapterinterface, but the Stack implementation doesn't require:
- Pre-filling (lazy initialization pattern)
- Timeout handling (synchronous operations)
- Actual locking (no concurrency)
This design is consistent with the adapter pattern and provides a lightweight alternative to the Swoole adapter.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Pools/Pool.php (2)
363-373: Race condition persists: connection creation still outside lock.The past review flagged this issue, which remains unresolved. After releasing the lock at line 367, lines 370-373 check
connectionsCreatedand callcreateConnection()without synchronization. Multiple concurrentdestroy()calls can race and exceed the$sizelimit.This is the same root cause as the race condition in
createConnection()(line 277). The fix should ensure the check-and-increment is atomic, as suggested in the earlier review comment.
378-393: Same race condition applies when destroying all connections.Lines 389-392 have the identical race condition as lines 370-373: the check and creation happen outside the lock. Multiple concurrent invocations can exceed the pool size limit.
The stable copy at line 379 correctly prevents iteration issues, but the concurrency issue with
connectionsCreatedremains.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
composer.json(1 hunks)src/Pools/Pool.php(9 hunks)tests/Pools/Scopes/PoolTestScope.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Pools/Scopes/PoolTestScope.php
- composer.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/Pools/Pool.php (4)
src/Pools/Adapter.php (6)
Adapter(5-27)fill(7-7)withLock(26-26)count(17-17)pop(15-15)push(9-9)src/Pools/Connection.php (5)
__construct(22-24)getID(29-32)Connection(10-105)setID(38-42)setPool(74-78)src/Pools/Adapter/Stack.php (5)
fill(12-17)withLock(36-39)count(31-34)pop(26-29)push(19-24)src/Pools/Adapter/Swoole.php (5)
fill(15-24)withLock(47-61)count(42-45)pop(33-39)push(26-31)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Pools/Scopes/PoolTestScope.php (1)
262-286: Timing assertions remain unreachable afterexpectException.The issue from the previous review still exists. Once
expectExceptionis set at line 276, PHPUnit stops execution when the exception is thrown at line 279. Lines 280-284 (timing verification) never execute.Restructure to verify timing before the exception terminates the test:
public function testPoolRetry(): void { $this->execute(function (): void { $this->setUpPool(); $this->poolObject->setReconnectAttempts(2); $this->poolObject->setReconnectSleep(2); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); - // Pool should be empty - $this->expectException(Exception::class); - $timeStart = \time(); - $this->poolObject->pop(); - $timeEnd = \time(); - - $timeDiff = $timeEnd - $timeStart; - - $this->assertGreaterThanOrEqual(4, $timeDiff); + try { + $this->poolObject->pop(); + $this->fail('Expected exception was not thrown'); + } catch (Exception $e) { + $timeEnd = \time(); + $timeDiff = $timeEnd - $timeStart; + $this->assertGreaterThanOrEqual(4, $timeDiff); + } }); }
🧹 Nitpick comments (1)
src/Pools/Adapter/Swoole.php (1)
37-37: Fix typo in comment.Minor typo: "occured" should be "occurred".
Apply this diff:
- // if pool is empty or timeout occured => result will be false + // if pool is empty or timeout occurred => result will be false
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Pools/Adapter.php(1 hunks)src/Pools/Adapter/Swoole.php(1 hunks)src/Pools/Pool.php(9 hunks)tests/Pools/Adapter/SwooleTest.php(1 hunks)tests/Pools/Scopes/PoolTestScope.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Pools/Adapter.php
- src/Pools/Pool.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Pools/Adapter/Swoole.php (3)
src/Pools/Adapter.php (6)
Adapter(5-27)fill(7-7)push(9-9)pop(15-15)count(17-17)withLock(26-26)src/Pools/Adapter/Stack.php (5)
fill(12-17)push(19-24)pop(26-29)count(31-34)withLock(36-39)src/Pools/Pool.php (3)
push(313-324)pop(225-267)count(331-335)
tests/Pools/Scopes/PoolTestScope.php (7)
src/Pools/Connection.php (2)
Connection(10-105)getResource(47-50)src/Pools/Pool.php (6)
Pool(15-439)count(331-335)pop(225-267)use(199-212)push(313-324)setTelemetry(171-190)src/Pools/Adapter.php (4)
Adapter(5-27)count(17-17)pop(15-15)push(9-9)tests/Pools/Base.php (2)
getAdapter(17-17)execute(23-23)tests/Pools/Adapter/StackTest.php (2)
getAdapter(10-13)execute(15-18)tests/Pools/Scopes/ConnectionTestScope.php (2)
getAdapter(11-11)execute(12-12)tests/Pools/Scopes/GroupTestScope.php (2)
getAdapter(11-11)execute(12-12)
🪛 PHPMD (2.15.0)
src/Pools/Adapter/Swoole.php
15-15: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
tests/Pools/Adapter/SwooleTest.php
150-150: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (6)
src/Pools/Adapter/Swoole.php (2)
13-14: Lock property is correctly used for compound operations.The past review comment flagging
$lockas unused is outdated. The lock is properly initialized infill()(line 21) and used inwithLock()(lines 51, 60) to provide mutual exclusion for compound operations. While Swoole Channels are coroutine-safe for individual push/pop operations, the lock is necessary for atomic multi-step operations like the count-and-check logic inPool::pop().Also applies to: 20-21, 48-62
51-51: Verify Swoole Lock timeout parameter unit.Ensure that
Lock::lockwait($timeout)expects timeout in seconds (matching the parameter's semantic meaning) rather than milliseconds. The Swoole documentation should confirm the expected unit.tests/Pools/Adapter/SwooleTest.php (2)
13-16: Adapter setup and execution wrapper look good.The
getAdapter()andexecute()implementations correctly instantiate the Swoole adapter and wrap callbacks inCoroutine\run()with proper exception handling.Also applies to: 18-37
39-118: Comprehensive test coverage for concurrent scenarios.The test suite thoroughly exercises the Swoole adapter under various concurrent conditions: race conditions, high concurrency, connection uniqueness, idle reuse, and stress testing. All tests properly skip when Swoole extension is not loaded and run within coroutine contexts.
Also applies to: 120-179, 181-244, 246-302, 304-359
tests/Pools/Scopes/PoolTestScope.php (2)
121-143: Test logic is now correct.The previous issue with unreachable assertions has been fixed. The test now correctly pops 4 remaining connections (lines 134-138) before setting
expectException(line 140) and attempting the final pop that should throw (line 141).
25-31: Test methods provide solid coverage of pool operations.The trait provides comprehensive test coverage for pool configuration (name, size, retry/reconnect settings), basic operations (pop, push, count, use), state checks (isEmpty, isFull), lifecycle (destroy, reclaim), and telemetry integration. The test structure is clean and follows consistent patterns.
Also applies to: 33-39, 41-47, 49-59, 61-67, 69-79, 81-87, 89-99, 101-107, 109-119, 145-157, 159-176, 178-192, 194-210, 212-224, 226-260, 288-319, 321-369
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Pools/Pool.php (2)
431-441: Telemetry metrics record identical values.Lines 435 and 440 both compute
activeConnections + idleConnections(sincepool->count()equalsidleConnections), resulting intelemetryPoolCapacityandtelemetryOpenConnectionsrecording the same value.Based on previous discussion, this duplication is intentional to track "current pool connections in current state" rather than maximum capacity. If this is the intended design, consider adding a comment explaining why both metrics track the same value.
17-17: Typo still present:SECODNSshould beSECONDS.The constant name still contains the typo that was previously flagged. This affects readability and should be corrected throughout the file (lines 242, 248, 376, 400).
Apply this diff:
- public const LOCK_TIMEOUT_IN_SECODNS = 3; + public const LOCK_TIMEOUT_IN_SECONDS = 3;Then update all usages:
- }, timeout: self::LOCK_TIMEOUT_IN_SECODNS); + }, timeout: self::LOCK_TIMEOUT_IN_SECONDS);
🧹 Nitpick comments (1)
src/Pools/Pool.php (1)
417-429: Consider renamingisFull()for clarity.The method name
isFull()conventionally suggests "at maximum capacity", but here it means "all connections are available (idle or not created)". While the comment at line 427 documents this intent, the naming could be clearer.Consider renaming to better express the semantics:
- public function isFull(): bool + public function isFullyAvailable(): bool { // Pool is full when all possible connections are available (idle or not created yet) return count($this->active) === 0; }Alternatively, keep the current naming if it aligns with the library's established conventions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Pools/Pool.php(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Pools/Pool.php (2)
src/Pools/Adapter.php (6)
Adapter(5-27)fill(7-7)withLock(26-26)count(17-17)pop(15-15)push(9-9)src/Pools/Adapter/Stack.php (5)
fill(12-17)withLock(36-39)count(31-34)pop(26-29)push(19-24)
🪛 GitHub Actions: CodeQL
src/Pools/Pool.php
[error] 245-245: PHPStan: Method Utopia\Pools\Pool::pop() should return Utopia\Pools\Connection but returns mixed. The static analysis found a type mismatch. Script './vendor/bin/phpstan analyse --level max src tests' exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (5)
src/Pools/Pool.php (5)
70-77: LGTM! Constructor properly implements adapter pattern.The constructor correctly accepts a
PoolAdapterand initializes it for lazy connection creation. The comment at line 74 clearly documents the intent.
278-310: LGTM! Connection creation pattern is correct.The
createConnection()method properly implements the helper pattern discussed in previous reviews. The increment at line 280 is protected because all call sites (lines 237, 373, 397) invoke this method withinwithLockcallbacks, ensuring atomic check-and-increment semantics.The retry logic with proper cleanup (decrements at lines 291, 299) ensures
connectionsCreatedremains accurate even when connection initialization fails.
316-327: LGTM! Push implementation is clean.The method correctly delegates to the adapter and maintains consistent state tracking by removing from
$this->activeand recording telemetry.
334-338: LGTM! Count logic correctly implements lazy initialization semantics.The calculation
pool->count() + (size - connectionsCreated)properly returns available capacity: idle connections plus not-yet-created connections. The comment clearly documents this behavior.
362-412: LGTM! Destroy method correctly implements synchronized destruction and replacement.The method properly addresses the concurrency concerns from previous reviews:
- Single connection (lines 366-381): Lock protects the decrement, unset, and replacement creation atomically.
- Multiple connections (lines 386-406): Uses
array_values()to create a stable copy (line 387), avoiding modification-during-iteration issues. Each replacement is created within its own lock for proper synchronization.
src/Pools/Pool.php
Outdated
| */ | ||
| class Pool | ||
| { | ||
| public const LOCK_TIMEOUT_IN_SECODNS = 3; |
There was a problem hiding this comment.
also is this value reasonable? should be the same was connectiontimeout?
There was a problem hiding this comment.
I search for similar connectiontimeout values but all the values were either retry attempts or sleep
so added another constant to explicitly track`
There was a problem hiding this comment.
Pull request overview
This pull request refactors the pool management system to support concurrent operations through an adapter pattern. The changes introduce lazy initialization of connections and thread-safe operations for Swoole-based coroutines while maintaining backward compatibility for single-threaded usage.
Key changes:
- Introduced an adapter pattern with
Stack(for single-threaded) andSwoole(for concurrent coroutine-based) implementations - Migrated from eager connection initialization to lazy initialization, creating connections only when needed
- Added mutex locking in the Swoole adapter to prevent race conditions during concurrent connection creation
- Reorganized tests into reusable trait-based scopes (PoolTestScope, GroupTestScope, ConnectionTestScope) that work across both adapters
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Pools/Adapter.php | New abstract base class defining the adapter interface for pool storage backends |
| src/Pools/Adapter/Stack.php | Simple array-based adapter for single-threaded pool operations |
| src/Pools/Adapter/Swoole.php | Coroutine-safe adapter using Swoole channels and mutex locks for concurrent operations |
| src/Pools/Pool.php | Refactored to use adapters, implement lazy initialization, and add locking for thread-safe connection creation |
| tests/Pools/Base.php | New abstract test base class that aggregates test scopes for both adapters |
| tests/Pools/Scopes/*.php | Reusable test traits ensuring consistent behavior across adapter implementations |
| tests/Pools/Adapter/StackTest.php | Concrete test class for Stack adapter using shared test scopes |
| tests/Pools/Adapter/SwooleTest.php | Concrete test class for Swoole adapter with additional concurrency stress tests |
| tests/Pools/*.php (removed) | Old monolithic test files removed in favor of trait-based approach |
| composer.json | Added swoole/ide-helper and ext-swoole suggestion for development |
| composer.lock | Updated with swoole/ide-helper dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Pools/Adapter/Swoole.php
Outdated
| { | ||
| $result = $this->pool->pop($timeout); | ||
|
|
||
| // if pool is empty or timeout occured => result will be false |
There was a problem hiding this comment.
There is a spelling error in the comment: "occured" should be "occurred".
There was a problem hiding this comment.
updated via doc string
src/Pools/Pool.php
Outdated
| return $this->pool->count() === 0; | ||
| } |
There was a problem hiding this comment.
The isEmpty() method implementation may not align with user expectations. According to the method's logic, the pool is only considered empty when there are no idle connections in the pool storage, but it doesn't account for connections that haven't been created yet (when connectionsCreated < size). Consider documenting this behavior or updating the implementation to check if count() === 0 which includes not-yet-created connections.
src/Pools/Adapter/Stack.php
Outdated
| public function withLock(callable $callback, int $timeout): mixed | ||
| { | ||
| return $callback(); | ||
| } |
There was a problem hiding this comment.
The withLock method doesn't actually provide any locking for the Stack adapter. While this is acceptable for non-concurrent usage, it should be documented that this implementation doesn't provide thread-safety guarantees, similar to how the Swoole adapter provides actual locking.
src/Pools/Pool.php
Outdated
| $this->telemetryOpenConnections->record($activeConnections + $idleConnections, $this->telemetryAttributes); | ||
| $this->telemetryPoolCapacity->record($activeConnections + $existingConnections, $this->telemetryAttributes); | ||
| $this->telemetryOpenConnections->record($openConnections, $this->telemetryAttributes); | ||
| $this->telemetryPoolCapacity->record($activeConnections + $this->pool->count(), $this->telemetryAttributes); |
There was a problem hiding this comment.
The telemetry capacity calculation is redundant. Line 440 calculates activeConnections + $this->pool->count(), which is the same as openConnections calculated on line 435. Consider reusing the $openConnections variable instead of recalculating the same value.
src/Pools/Pool.php
Outdated
| $newConnection = $this->pool->withLock(function () use ($connection) { | ||
| $this->connectionsCreated--; | ||
| unset($this->active[$connection->getID()]); |
There was a problem hiding this comment.
The connectionsCreated counter is decremented before verifying the connection should be destroyed. If the connection being destroyed is not actually in the active array, this will incorrectly decrement the counter. The unset operation on line 369 will silently succeed even if the connection ID doesn't exist in the array. Consider checking if the connection exists in active before decrementing the counter.
| public function pop(int $timeout): mixed | ||
| { | ||
| $result = $this->pool->pop($timeout); | ||
|
|
||
| // if pool is empty or timeout occured => result will be false | ||
| return $result; | ||
| } |
There was a problem hiding this comment.
The pop method should document its timeout behavior more clearly. The method returns false on timeout or when the pool is empty, but this behavior is only mentioned in a comment on line 37. The PHPDoc should explicitly document the return type possibilities including false for timeout scenarios.
src/Pools/Pool.php
Outdated
| $newConnection = $this->pool->withLock(function () { | ||
| if ($this->pool->count() === 0 && $this->connectionsCreated < $this->size) { | ||
| $connection = $this->createConnection(); | ||
| $this->active[$connection->getID()] = $connection; | ||
| return $connection; | ||
| } | ||
| return null; | ||
| }, timeout: self::LOCK_TIMEOUT_IN_SECODNS); |
There was a problem hiding this comment.
The lock is acquired on every iteration of the retry loop even when the pool might not be empty. This can cause performance degradation under high concurrency. Consider checking $this->pool->count() before acquiring the lock to avoid unnecessary lock contention when connections are available in the pool.
src/Pools/Pool.php
Outdated
| */ | ||
| class Pool | ||
| { | ||
| public const LOCK_TIMEOUT_IN_SECODNS = 3; |
There was a problem hiding this comment.
There is a typo in the constant name: "SECODNS" should be "SECONDS".
…ency handling and maintain pool size
…c for connection counting
src/Pools/Adapter/Stack.php
Outdated
| * @param int $timeout Ignored. | ||
| * @return mixed The value returned by the callback. | ||
| */ | ||
| public function withLock(callable $callback, int $timeout): mixed |
There was a problem hiding this comment.
I would suggest synchronized for a name, because this doesn't actually lock on Stack implemenation.
| $this->pool = new Channel($size); | ||
|
|
||
| // Initialize lock for thread-safe operations | ||
| $this->lock = new Lock(SWOOLE_MUTEX); |
There was a problem hiding this comment.
Will this lock the whole worker? Or just the coroutine? Which is desired?
There was a problem hiding this comment.
This mutex is shared across all coroutines in the worker process. When one coroutine acquires it, the entire worker is blocked until the lock is released. This ensures atomic updates (e.g., incrementing/decrementing counters or registering/deregistering active connections)
so all operations on the pool will be synchronized
src/Pools/Pool.php
Outdated
| $this->pool = array_fill(0, $this->size, true); | ||
| $this->pool = $adapter; | ||
| // Initialize empty channel (no pre-filling for lazy initialization) | ||
| $this->pool->fill($this->size, null); |
There was a problem hiding this comment.
Is this standard? Not sure what best practice is here
Lazy initialisation vs prefilled connections
There was a problem hiding this comment.
Pre filled was the previous the approach where we maintain the true based placeholders.
Lazy approach is inspired from the swoole connection pool implementation
It only consists of the connection object
| * @return $this<TResource> | ||
| */ | ||
| public function destroy(?Connection $connection = null): static | ||
| private function destroyConnection(?Connection $connection = null): static |
There was a problem hiding this comment.
does this actually allow us to destroy the connection? e.g. close a database connection?
There was a problem hiding this comment.
Didn't get you here. You meant the destroyConnection? So destroy was using the same logic twice plus the telemetry.
So now destroy is calling the destroyConnection as a recursive process and will update the telemetry
src/Pools/Pool.php
Outdated
| */ | ||
| class Pool | ||
| { | ||
| public const LOCK_TIMEOUT_IN_SECONDS = 3; |
There was a problem hiding this comment.
Where does this number come from? Should it be configurable?
There was a problem hiding this comment.
Sure will make it make configurable with a setter instead of a constant
src/Pools/Adapter.php
Outdated
|
|
||
| abstract class Adapter | ||
| { | ||
| abstract public function fill(int $size, mixed $value): static; |
There was a problem hiding this comment.
Probably fill should accept callable $value instead
There was a problem hiding this comment.
fill() was a bit misleading in this context. The pool does not eagerly create or insert values; connections are created lazily on demand. Because of that, the $value parameter was never actually used as a concrete value.
To better reflect the behavior, this has been changed to initialize(int $size), which only sets up the pool capacity and internal state.
…eplace 'withLock' with 'synchronized' for consistency across adapters.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Pools/Pool.php (1)
350-361: Consider synchronizing theunsetoperation on$active.The
pushmethod removes the connection from$active(line 355) without synchronization, whilepopadds to it with synchronization (line 275). This inconsistency could lead to race conditions in concurrent environments.🔎 Suggested fix:
public function push(Connection $connection): static { try { // Push the actual connection back to the pool $this->pool->push($connection); - unset($this->active[$connection->getID()]); + $this->pool->synchronized(function () use ($connection) { + unset($this->active[$connection->getID()]); + }, timeout: $this->getSynchronizationTimeout()); return $this; } finally { $this->recordPoolTelemetry(); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Pools/Adapter.php(1 hunks)src/Pools/Adapter/Stack.php(1 hunks)src/Pools/Adapter/Swoole.php(1 hunks)src/Pools/Pool.php(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Pools/Adapter.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Pools/Adapter/Swoole.php (2)
src/Pools/Adapter.php (6)
Adapter(5-27)initialize(7-7)push(9-9)pop(15-15)count(17-17)synchronized(26-26)src/Pools/Pool.php (3)
push(350-361)pop(250-308)count(368-372)
src/Pools/Pool.php (3)
src/Pools/Adapter.php (6)
Adapter(5-27)initialize(7-7)synchronized(26-26)count(17-17)pop(15-15)push(9-9)src/Pools/Connection.php (4)
__construct(22-24)Connection(10-105)setPool(74-78)destroy(97-104)src/Pools/Adapter/Stack.php (5)
initialize(22-26)synchronized(64-67)count(49-52)pop(44-47)push(28-33)
🪛 PHPMD (2.15.0)
src/Pools/Adapter/Stack.php
22-22: Avoid unused parameters such as '$size'. (undefined)
(UnusedFormalParameter)
44-44: Avoid unused parameters such as '$timeout'. (undefined)
(UnusedFormalParameter)
64-64: Avoid unused parameters such as '$timeout'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (19)
src/Pools/Adapter/Swoole.php (4)
1-22: LGTM! Clean adapter initialization with proper Swoole primitives.The adapter correctly initializes a
Channelwith the specified capacity and aSWOOLE_MUTEXlock for synchronized operations. The implementation aligns well with the abstractAdaptercontract.
24-29: LGTM!Simple and correct delegation to the underlying channel.
38-47: LGTM! Proper timeout handling and defensive count implementation.The
popcorrectly delegates to the channel with the timeout parameter, andcountdefensively handles potential non-int returns fromlength().
61-74: LGTM! Correct lock acquisition with timeout and proper cleanup.The
synchronizedmethod correctly:
- Acquires the lock with a timeout
- Throws a clear exception if lock acquisition fails
- Uses
finallyto guarantee unlock even on exceptionssrc/Pools/Adapter/Stack.php (5)
12-26: LGTM! Clear documentation of API compatibility behavior.The PHPDoc clearly explains that
$sizeis ignored for API compatibility. The static analysis warning about unused$sizeis expected here since the Adapter interface requires the parameter signature.
28-33: LGTM!Simple array append for connection storage.
35-47: LGTM! Well-documented timeout behavior.The PHPDoc clearly states that blocking is not supported and the timeout is ignored.
49-52: LGTM!Straightforward count implementation.
54-67: LGTM! Clear documentation of non-locking behavior.The PHPDoc explicitly states this implementation does not provide mutual exclusion, which is appropriate for non-concurrent usage scenarios.
src/Pools/Pool.php (10)
43-47: LGTM! New synchronized timeout configuration.The
synchronizedTimeoutproperty provides configurable lock timeout control for adapters that support it.
54-58: LGTM! Connection tracking for lazy initialization.The
connectionsCreatedcounter enables lazy connection creation while respecting pool size limits.
68-81: LGTM! Adapter-backed constructor with lazy initialization.The constructor correctly wires the adapter and initializes it without pre-filling connections.
171-190: LGTM! Clean getter/setter for synchronization timeout.Good documentation explaining the behavior for adapters that don't support locking.
258-284: Race condition properly addressed with check-increment-create pattern.The implementation follows the correct pattern discussed in past reviews:
- Lock: check + increment atomically (lines 263-269)
- Unlock
- Create connection outside lock (line 273)
- On failure: lock + decrement (lines 279-281)
This prevents multiple coroutines from exceeding the pool size limit.
316-344: LGTM! Clean connection creation helper with retry logic.The
createConnectionmethod correctly:
- Implements retry logic for connection failures
- Assigns a unique ID if missing
- Associates the connection with the pool
Note: The counter increment/decrement is handled by callers, which is the correct pattern as discussed in past reviews.
368-372: LGTM! Updated count semantics for lazy initialization.The count now correctly returns the total available connections: those idle in the pool plus those not yet created.
396-427: Well-structured destroy with synchronized counter management.The
destroyConnectionmethod correctly:
- Atomically decrements counter and checks if replacement is needed (lines 399-407)
- Creates replacement outside the lock to avoid blocking
- Decrements on failure (lines 413-415)
- Iterates over a copy of active connections to avoid modification during iteration (line 422)
445-457: VerifyisFullsemantics align with pool behavior.The
isFullmethod returnstruewhencount($this->active) === 0, meaning no connections are currently in use. This is different from traditional "pool is full" semantics (all connections created). The naming might be confusing—consider if this should be namedisIdle()orhasNoActiveConnections()instead.The current implementation means:
isEmpty(): No connections are available (all in use and at capacity)isFull(): No connections are active (all idle or not created)Is this the intended behavior? If so, consider adding a docblock to clarify.
459-469: Telemetry now correctly usesconnectionsCreatedfor capacity.This addresses the earlier confusion about
telemetryPoolCapacity. RecordingconnectionsCreatedshows how many connections have actually been instantiated, which is useful for monitoring lazy initialization behavior.
…rrency issue during destroy
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/Pools/Pool.php (1)
352-363: Inconsistent synchronization:$this->activemodification is unprotected.Line 357 modifies
$this->activeoutside a synchronized block, whilepop()correctly wraps similar modifications insynchronized()calls (lines 274-276, 297-299). In Swoole environments, concurrentpush()andpop()calls could race on the$this->activearray.🔎 Suggested fix:
public function push(Connection $connection): static { try { // Push the actual connection back to the pool $this->pool->push($connection); - unset($this->active[$connection->getID()]); + $this->pool->synchronized(function () use ($connection) { + unset($this->active[$connection->getID()]); + }, timeout: $this->getSynchronizationTimeout()); return $this; } finally { $this->recordPoolTelemetry(); } }
♻️ Duplicate comments (2)
src/Pools/Pool.php (2)
461-471: Telemetry capacity metric name may be misleading.Line 470 records
connectionsCreatedaspool.connection.capacity.count. Based on the previous discussion, you intend this to represent "current pool connections in current state." However, "capacity" typically implies the maximum limit, which would be$this->size. Consider renaming to something likepool.connection.created.countorpool.connection.total.countto better reflect what's being measured.
398-420: Potential issue: counter decremented without verifying connection exists in$this->active.The
connectionsCreatedcounter is decremented (line 402) before verifying that the connection actually exists in$this->active. IfdestroyConnection()is called with a connection that isn't tracked, the counter becomes inconsistent with actual state.🔎 Suggested fix:
if ($connection !== null) { $shouldCreate = $this->pool->synchronized(function () use ($connection) { + if (!isset($this->active[$connection->getID()])) { + return false; // Connection not tracked, nothing to destroy + } $this->connectionsCreated--; unset($this->active[$connection->getID()]); if ($this->connectionsCreated < $this->size) { $this->connectionsCreated++; return true; }; return false; }, timeout: $this->getSynchronizationTimeout());
🧹 Nitpick comments (1)
src/Pools/Pool.php (1)
187-190: Consider adding PHPDoc for consistency.The
setSynchronizationTimeoutmethod has comprehensive documentation, butgetSynchronizationTimeoutlacks a PHPDoc block. For consistency with other getter/setter pairs in this class, consider adding one.🔎 Suggested fix:
+ /** + * Get the lock timeout for adapters that support synchronized locking. + * + * @return int Synchronized lock timeout in seconds. + */ public function getSynchronizationTimeout(): int { return $this->synchronizedTimeout; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Pools/Pool.php(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Pools/Pool.php (4)
src/Pools/Adapter.php (6)
Adapter(5-27)initialize(7-7)synchronized(26-26)count(17-17)pop(15-15)push(9-9)src/Pools/Connection.php (6)
__construct(22-24)getID(29-32)Connection(10-105)setID(38-42)setPool(74-78)destroy(97-104)src/Pools/Adapter/Stack.php (5)
initialize(22-26)synchronized(64-67)count(49-52)pop(44-47)push(28-33)src/Pools/Adapter/Swoole.php (5)
initialize(15-22)synchronized(61-74)count(43-47)pop(38-41)push(24-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (7)
src/Pools/Pool.php (7)
43-57: LGTM!The new properties for synchronization timeout and connection tracking are well-structured. The default timeout of 3 seconds is reasonable, and the
connectionsCreatedcounter supports the lazy initialization pattern.
74-81: LGTM!The constructor correctly initializes the adapter with lazy initialization. The change to require
PoolAdapteras the first parameter is a breaking change for existing consumers—ensure this is documented in migration notes.
258-284: Well-implemented synchronization pattern.The connection creation logic correctly follows the atomic check-and-increment pattern discussed in previous reviews:
- Lock: check condition + increment counter (lines 263-269)
- Unlock: create connection outside lock (line 273)
- On failure: lock + decrement (lines 279-281)
This prevents the race condition where multiple coroutines could exceed the pool size limit.
318-346: LGTM!The
createConnection()helper cleanly encapsulates retry logic and connection setup. The increment responsibility has been correctly moved to callers who hold the lock, addressing the race condition concerns from previous reviews.
370-374: LGTM!The
count()method now represents available capacity (idle + not-yet-created), which aligns with the lazy initialization model. The inline comment clarifies the semantics.
435-442: LGTM!The public
destroy()method cleanly delegates todestroyConnection()while ensuring telemetry is always recorded.
447-459: LGTM!The
isEmpty()andisFull()semantics are consistent with the lazy initialization model:
isEmpty(): No connections available (all created and all in use)isFull(): All connections available (none currently in use)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Pools/Scopes/PoolTestScope.php (1)
262-286: Timing assertions are still unreachable afterexpectException.Despite the previous review comment being marked as addressed, the timing verification is still broken. When
expectExceptionis called at line 276, PHPUnit stops test execution when the exception is thrown at line 279. Lines 280-284 never execute, so the retry timing is never verified.🔎 Proposed fix using try-catch
public function testPoolRetry(): void { $this->execute(function (): void { $this->setUpPool(); $this->poolObject->setReconnectAttempts(2); $this->poolObject->setReconnectSleep(2); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); - // Pool should be empty - $this->expectException(Exception::class); - $timeStart = \time(); - $this->poolObject->pop(); - $timeEnd = \time(); - - $timeDiff = $timeEnd - $timeStart; - - $this->assertGreaterThanOrEqual(4, $timeDiff); + try { + $this->poolObject->pop(); + $this->fail('Expected exception was not thrown'); + } catch (Exception $e) { + $timeEnd = \time(); + $timeDiff = $timeEnd - $timeStart; + $this->assertGreaterThanOrEqual(4, $timeDiff); + } }); }
🧹 Nitpick comments (1)
tests/Pools/Scopes/PoolTestScope.php (1)
428-428: Consider using$_for intentionally unused parameters.The
$resourceparameter in these closures is required by theuse()method signature but isn't used in these specific test cases. Using$_instead signals to both readers and static analysis tools that the parameter is intentionally unused.🔎 Proposed changes
try { - $pool->use(function ($resource) use (&$attempts) { + $pool->use(function ($_) use (&$attempts) { $attempts++; throw new Exception("Persistent failure"); }, 2); // Allow up to 2 retries (3 total attempts)try { - $pool->use(function ($resource) use (&$attempts) { + $pool->use(function ($_) use (&$attempts) { $attempts++; throw new Exception("First attempt failure"); }); // No retries (default)Also applies to: 449-449
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Pools/Pool.phptests/Pools/Scopes/PoolTestScope.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Pools/Pool.php
🧰 Additional context used
🪛 GitHub Actions: CodeQL
tests/Pools/Scopes/PoolTestScope.php
[error] 433-433: PHPStan: Unreachable statement - code above always terminates.
[error] 454-454: PHPStan: Unreachable statement - code above always terminates.
🪛 PHPMD (2.15.0)
tests/Pools/Scopes/PoolTestScope.php
428-428: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
449-449: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (7)
tests/Pools/Scopes/PoolTestScope.php (7)
10-13: LGTM!The abstract method declarations are consistent with the test architecture and properly delegate adapter instantiation and execution context to concrete test classes.
20-23: LGTM!The setup helper creates a pool with deterministic behavior suitable for testing.
25-119: LGTM!The getter and setter tests are well-structured and provide comprehensive coverage of pool configuration properties.
121-260: LGTM!The pool operation tests correctly verify pop, push, use, count, reclaim, isEmpty, and isFull behaviors. The testPoolPop fix from the previous review has been properly applied.
288-369: LGTM!The destroy and telemetry tests correctly verify pool lifecycle management and observability integration.
371-418: LGTM!The retry success scenarios correctly validate that the pool retries failed operations and eventually succeeds when possible.
420-494: LGTM!The retry failure tests correctly verify exhaustion behavior and connection lifecycle during retries.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Pools/Scopes/PoolTestScope.php (1)
262-286: Critical: Timing assertions are still unreachable.Despite the past review marking this as addressed, the test structure remains incorrect.
expectExceptionat line 276 causes PHPUnit to stop when the exception is thrown at line 279. Lines 280-284 never execute, so retry timing is never verified.The fix requires using try-catch:
🔎 Proposed fix
public function testPoolRetry(): void { $this->execute(function (): void { $this->setUpPool(); $this->poolObject->setReconnectAttempts(2); $this->poolObject->setReconnectSleep(2); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); $this->poolObject->pop(); - // Pool should be empty - $this->expectException(Exception::class); - $timeStart = \time(); - $this->poolObject->pop(); - $timeEnd = \time(); - - $timeDiff = $timeEnd - $timeStart; - - $this->assertGreaterThanOrEqual(4, $timeDiff); + try { + $this->poolObject->pop(); + $this->fail('Expected exception was not thrown'); + } catch (Exception $e) { + $timeEnd = \time(); + $timeDiff = $timeEnd - $timeStart; + $this->assertGreaterThanOrEqual(4, $timeDiff); + } }); }
🧹 Nitpick comments (1)
src/Pools/Pool.php (1)
479-489: Telemetry capacity duplicates open connections.Both
telemetryPoolCapacity(line 488) andtelemetryOpenConnections(line 487) record identical values:
openConnections = activeConnections + idleConnections(line 483)telemetryPoolCapacityrecordsconnectionsCreated(line 488)Since
destroyConnection()decrementsconnectionsCreatedand connections are either active or idle,connectionsCreatedalways equalsactiveConnections + idleConnections. This makes the metrics redundant.Consider either:
- Recording
$this->size(the configured maximum) to track capacity as a constant limit, or- Removing one of the duplicate metrics
This allows monitoring systems to distinguish between current usage (
openConnections) and configured capacity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Pools/Pool.phptests/Pools/Scopes/PoolTestScope.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/Pools/Scopes/PoolTestScope.php
428-428: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
448-448: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
ArnabChatterjee20k
force-pushed
the
dat-966
branch
from
a073ae4 to
c32a92a
Compare
Problem addressed
When using pools in concurrent environment like in swoole, there is a chance that two coroutines are operating(push,pop,etc) at the same bypassing the checks and a classic case was coming when there is one idle connection in the pool and two coroutines popping at the same time leading to pool empty
Solution introduced
A new adapter driven approach.
Adapters
Stack Adapter
Swoole Adapter
The pool logic remains the same; concurrency behavior is delegated to the adapter.
Breaking Changes
Before
After
Callers must now explicitly choose the adapter based on the runtime environment.
Why Not Use Swoole’s Built-in ConnectionPool?
Reference:
https://github.com/swoole/library/blob/master/src/core/ConnectionPool.php
Swoole ConnectionPool
Utopia Pool
Lazy vs Prefilled Connections
Previous Approach
trueindicated “can create”Problems with this approach:
Current Approach (Lazy)
connectionsCreatedcounterLocks vs Channels (Design Decisions)
Shared Critical State
pool)active)connectionsCreated)Stack Adapter
Swoole Adapter
pop()andpush()activeandconnectionsCreatedThis design is inspired by Swoole’s ConnectionPool, with additional ownership tracking.
Why
synchronized(Notlock)We uses
synchronizedinstead oflockbecause:Coroutine vs Worker Blocking
In the Swoole adapter:
This behavior is intentional and desired:
Deadlock Avoidance
Timeouts
synchronizedTimeoutdefaults to 3 seconds-1) are discouragedNo Retries Inside Synchronized Blocks
The following pattern is used:
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.