Open intellent opened 2 years ago
Your trying to put the connection back twice... Once on error and once in your finally block.
Your trying to put the connection back twice... Once on error and once in your finally block.
Yep, I know. But I propose this should not result in a non-catchable fatal error.
Well... That's a logic problem, not something to do with Swoole, PHP or any programming language...
And your not actually catching the exception. You need to wrap your finally stuff in another try/catch if you want to catch it. ( Better to fix your logic tho)
Dead lock is also a fatal error in Golang. If we drop the dead-lock error, you added objects to the pool that exceeded the capacity of pool and you did not close the pool before your process exit, then your process would be stuck forever without any error infomation.
I’m not asking you to drop the error. Just turning it into a catchable Exception and not killing the entire application would be nice.
The reason why it was designed as fatal error is that you have to fix your code, it should not be caught.
@intellent
I did my own experiments, sample code is different, but seems consistent with OP observation:
after the $pool->put($pdo);
line, I add a new line in a few different ways:
Here are 3 ideas at application level:
Can someone please share some insight as why put(conn) is not idempotent ?
@yespire Thanks for testing this. I agree. put(conn)
could as well be idempotent. But I wouldn’t consider it a must. Throwing a catchable warning would be fine by me, as well.
Either way, a repeated put(conn)
shouldn’t result in a dead Swoole process. This was my intention when I created this issue. So thanks for helping with clarification.
And btw: Your 3 ideas are exactly what I ended up implementing. However, having to build a Pool around a Pool doesn’t feel right.
It doesn't make sense not to have an error/exception if you put back the same connection multiple times, since put
adds it to a list. So logically you end up with the same item twice in said list. So when you pop an item off, you might still have it in your list.
If you had something like $pool->set($id, $con)
, which would add it to a map on index $id
, then it would make sense not to throw an error.
The way we do it, is that we use the DB class more or less like a singleton which abstracts away all the pool related things so you don't have to worry about it in the rest of the application.
Pseudo-code Example:
class DB
{
...
private static function pool_exec(callable $function): mixed {
self::$db_pool ??= new ConnectionPool(function () {return new dbMySqli(...);}, 99); # Init the pool
$db = self::$db_pool->get(); # Get the connection
$exception = null;
try {
$r = $function($db); # run it, and catch exceptions
} catch (Throwable $e) {
$exception = $e;
}
self::$db_pool->put($db); # put the connection back
$exception and throw $exception; # throw the exceptions, if found
return $r ?? null; # return something
}
public static function selectRow(string $sql, array $args = null, bool $MYSQL_ASSOC = true): array {
return self::pool_exec(function (dbMySqli $db) use ($sql, $args, $MYSQL_ASSOC) {
return $db->selectRow($sql, $args, $MYSQL_ASSOC); # This is some function that uses said $db connection.
});
}
}
# anywhere in coroutines.
$row = DB::selectRow("SELECT * FROM foo WHERE a = ? AND b = ? AND c = ?", [1,2,3]);
@ValiDrv
It doesn't make sense not to have an error/exception if you put back the same connection multiple times
@intellent
Either way, a repeated put(conn) shouldn’t result in a dead Swoole process.
I need to correct myself for my first comment. I was under the impression the process is unresponsive. I looked carefully and did more experiments with try catch. Here is my corrected observations / thinking:
try {
$conn = pool->get();
...
$stmt = $conn->prepare_statement()
$stmt->execute() // exception
...
pool->put($conn);
} catch (\Throwable $e) {
// notify dev
}
throw exception from $pool->put() or $poolAppWrapper->put() are not good idea, because exceptions are expensive, bad for performance, trace can be very heavy to generate for complex app
getting PDOException from (1). is not optimal outcome either ~ double put same connection would lead to a non-recoverable situation, there is no built in way to fix the pool, I think rebuilding the pool can be done, but if really come to this, it's far easier to develop and maintain preventative logic ~ so when catching exception, the only thing left to do is sending out alerts to the developers
I would argue that there are only 2 reasonable choices: a. implement idempotent put() in $pool or $poolAppWrapper b. do nothing (since I don't think double put can happen in my code, so this is my case)
1. What did you do? If possible, provide a simple script for reproducing the error.
I’m returning connections back to the pool, if exceptions happen. This may happen in the main function or some sub-routine.
2. What did you expect to see?
Well, a working application. If I try to put back a connection to the pool, which is already in the pool, this should either be silently ignored or throw a catchable exception/warning.
3. What did you see instead?
The system just crashes and doesn’t respond anymore. The logs show
4. What version of Swoole are you using (show your
php --ri swoole
)?5. What is your machine environment used (show your
uname -a
&php -v
&gcc -v
) ?