Release redis connection immediately when using transaction or pipeline with callbacks.#7394
Release redis connection immediately when using transaction or pipeline with callbacks.#7394limingxinleo merged 46 commits into
transaction or pipeline with callbacks.#7394Conversation
|
自己加个并发测试下 |
…eleased by MultiExec
@limingxinleo I added the concurrent tests you requested. I modified The tests show: with only 3 connections, we can handle 20 concurrent pipeline callbacks successfully. This proves the fix prevents connection pool exhaustion. Technical note: I had to reorder the mock setup because |
|
给你加了个单测,处理下 |
@limingxinleo Fixed. The connection is now only released immediately if we are still on the default database. |
| if (! $connection->getDatabase() || $connection->getDatabase() === $connection->getConfig()['db']) { | ||
| Context::set($contextKey, null); | ||
| $connection->release(); | ||
| } |
There was a problem hiding this comment.
@limingxinleo I did it that way because it's the same logic as here: https://github.com/hyperf/redis/blob/25667386bb7d0bb1307e9e65d91ca8f80cf68b14/src/RedisConnection.php#L181
I couldn't think of a better approach. Maybe you have a better idea.
There was a problem hiding this comment.
@limingxinleo I found an issue with the tests. The coroutines were completing too quickly so the tests didn't catch the concurrency problem.
I added sleep(1) after the pipeline and transaction callbacks to simulate real work that happens in long-running production code. Now the tests are failing: https://github.com/hyperf/hyperf/actions/runs/15482108363/job/43589683384?pr=7394#step:7:23
I think this is what happens when MultiExec::pipeline(callback) runs:
$this->__call('pipeline', [])- empty arguments array!isset($arguments[0])returnstrueshouldUseSameConnection()returnstrue- Connection uses
defer()instead of immediate release
So connections are held until coroutine ends, not released immediately after callbacks.
My original approach worked because it released connections immediately in the finally block of the MultiExec trait, right after the callback completed.
|
@limingxinleo I refactored my original approach to make it cleaner. I extracted common logic into Using All tests pass. What do you think? |
|
又给你加了个单测,处理下 |
@limingxinleo Code is working and Redis tests are passing, but there is one remaining PHPStan error. Can you help with that? |
|
容我再想想 |
transaction or pipeline with callbacks.
In coroutines, Redis connections are released immediately after all commands except multi-exec commands like
pipeline(). This causes rapid Redis connection pool exhaustion when usingpipeline()in long-running coroutines (see: #7351).I don't think there's any reason to hold the connection after a multi-exec callback has finished executing. This PR releases Redis connections immediately after
exec()is called in pipeline/transaction operations that use callbacks (rather than deferring the release until the coroutine completes).Note: This improvement only applies to callback-based usage:
Manual usage continues to use the existing deferred cleanup:
Changes:
MultiExectrait to explicitly release connections infinallyblocks after pipeline/transaction callback executioncontextKeytracking toRedisConnectionto properly manage connection contextRedis::__call()to prevent double-releasing connectionsReal-world Impact:
With this change, I successfully ran 100+ concurrent long-running coroutines using Redis
pipeline()callbacks, with the Redis poolmax_connectionsset to just10, and experienced no connection pool exhaustion. Previously, the same code would exhaust the connection pool after only 10 coroutines, creating a scaling problem under high load. This is a significant improvement in connection efficiency.Benefits: