swoole / library

📚 Swoole Library
https://wiki.swoole.com/#/library
Apache License 2.0
232 stars 59 forks source link

Fixed the ability to call reset to not PDOProxy object #170

Closed DevZer0x00 closed 4 months ago

deminy commented 4 months ago

@DevZer0x00 Thanks for submitting the PR. Could you please let me know how you noticed the issue? Is there a way to (easily) reproduce it? I'm asking these because of two reasons:

  1. I'd like to reproduce it and add test cases for it after confirmed/merged.
  2. Similar issue applies to other connection pools as well, not just PDO. When method ConnectionPool::get() fails, people could make a retry by calling the same method again. I'm wondering if there is any better way to deal with this type of failures.
DevZer0x00 commented 4 months ago

@deminy

I added a test.

  1. I’ll also write down in text how a problematic situation arises - if requests have selected all available connections from the pool and if the timeout has expired and there are no available connections, then $pool->get($timeout) - a Boolean value is returned, just below the code $pdo- >reset(); which causes the error "Call to a member function reset() on bool"
  2. Yes, it is advisable to throw an exception in the connection pool so that, depending on the situation, you can make a decision in the client code whether to try to get a connection from the pool or throw an error

If you don't mind, I can take the exception to the ConnectionPool

deminy commented 4 months ago

The PR has been merged.

I made a change by returning a value of false when failed to get a PDO connection from the pool. This is to make the behavior of the ConnectionPool::get() methods consistent across pools of different connection objects (PDO, mysqli, etc).

For method ConnectionPool::get(), returning a value of false means that the pool is at full capacity, yet all connections are currently in use.

Please feel free to let me know any questions. Thanks @DevZer0x00