-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Release redis connection immediately when using transaction or pipeline with callbacks.
#7394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Release redis connection immediately when using transaction or pipeline with callbacks.
#7394
Conversation
|
自己加个并发测试下 |
…eleased by MultiExec
a6930cf to
f3b2868
Compare
@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. |
src/redis/src/Traits/MultiExec.php
Outdated
| if (! $connection->getDatabase() || $connection->getDatabase() === $connection->getConfig()['db']) { | ||
| Context::set($contextKey, null); | ||
| $connection->release(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
你这个也太硬编码了吧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@binaryfire 我改了,你试试
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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: