Skip to content

Conversation

@binaryfire
Copy link
Contributor

@binaryfire binaryfire commented May 27, 2025

In coroutines, Redis connections are released immediately after all commands except multi-exec commands like pipeline(). This causes rapid Redis connection pool exhaustion when using pipeline() 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:

$redis->pipeline(function($pipe) { ... }); // ✓ Immediate release after callback completes

Manual usage continues to use the existing deferred cleanup:

$pipe = $redis->pipeline(); // $callback is null, still released at coroutine end
$pipe->set('key', 'value');
$pipe->exec();

Changes:

  • Modified MultiExec trait to explicitly release connections in finally blocks after pipeline/transaction callback execution
  • Added contextKey tracking to RedisConnection to properly manage connection context
  • Updated defer logic in Redis::__call() to prevent double-releasing connections

Real-world Impact:

With this change, I successfully ran 100+ concurrent long-running coroutines using Redis pipeline() callbacks, with the Redis pool max_connections set to just 10, 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:

  • Prevents connection pool exhaustion in long-running coroutines when using multi-exec commands with callbacks
  • Maintains full backwards compatibility for existing code
  • No breaking changes - connections are still properly managed for all use cases

@binaryfire binaryfire changed the title Release redis connection immediately after multi-exec commands Release redis connection immediately after multi-exec closures May 30, 2025
@binaryfire binaryfire changed the title Release redis connection immediately after multi-exec closures Release redis connection immediately after multi-exec callbacks May 30, 2025
@limingxinleo
Copy link
Member

自己加个并发测试下

@binaryfire binaryfire force-pushed the release-redis-connection-after-multi-exec branch from a6930cf to f3b2868 Compare June 5, 2025 07:03
@binaryfire
Copy link
Contributor Author

binaryfire commented Jun 5, 2025

自己加个并发测试下

@limingxinleo I added the concurrent tests you requested.

I modified getRedis() to accept a $maxConnections parameter so we can test with limited connection pools.

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 RedisPool needs Channel to be mocked first.

@limingxinleo
Copy link
Member

给你加了个单测,处理下

@binaryfire
Copy link
Contributor Author

给你加了个单测,处理下

@limingxinleo Fixed. The connection is now only released immediately if we are still on the default database.

Comment on lines 73 to 76
if (! $connection->getDatabase() || $connection->getDatabase() === $connection->getConfig()['db']) {
Context::set($contextKey, null);
$connection->release();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

你这个也太硬编码了吧

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@binaryfire 我改了,你试试

Copy link
Contributor Author

@binaryfire binaryfire Jun 6, 2025

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:

  1. $this->__call('pipeline', []) - empty arguments array
  2. !isset($arguments[0]) returns true
  3. shouldUseSameConnection() returns true
  4. 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.

@binaryfire
Copy link
Contributor Author

binaryfire commented Jul 2, 2025

@limingxinleo I refactored my original approach to make it cleaner.

I extracted common logic into releaseContextConnection() in the Redis class and added isUsingDefaultDatabase() to RedisConnection. This removes code duplication. And I simplified MultiExec.

Using !isset($arguments[0]) in shouldUseSameConnection() had an issue: it always returns true because MultiExec passes an empty array, so connections were still being deferred. This approach lets the connection defer normally during pipeline setup, then manually releases it in the finally block after the callback completes.

All tests pass. What do you think?

@limingxinleo
Copy link
Member

又给你加了个单测,处理下

@binaryfire
Copy link
Contributor Author

又给你加了个单测,处理下

@limingxinleo Code is working and Redis tests are passing, but there is one remaining PHPStan error. Can you help with that?

@limingxinleo
Copy link
Member

容我再想想

@limingxinleo limingxinleo changed the title Release redis connection immediately after multi-exec callbacks Release redis connection immediately when using transaction or pipeline with callbacks. Jul 3, 2025
@limingxinleo limingxinleo merged commit e9fb04f into hyperf:master Jul 3, 2025
82 of 83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants