PDOPool: Putting an already returned connection back results in deadlock

[intellent]

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.

class PoolExample
{
    protected $pool;

    public function main()
    {
        $this->pool = new PDOPool(…);

        $connection = $this->pool->get();

        try {
            // Do something with $connection
            $this->subroutine($connection);
            // Do something else
        } finally {
            $this->pool->put($connection);
        }
    }


    protected function subroutine(PDOProxy $connection)
    {
        try {
            // Do something with connection that throws an exception (e.g. invalid SQL syntax)
        } catch (Exception $e) {
            $this->pool->put($connection);
            throw $e;
        }
    }
}

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

[FATAL ERROR]: all coroutines (count: 1) are asleep - deadlock!

4. What version of Swoole are you using (show your php --ri swoole)?

swoole

Swoole => enabled
Author => Swoole Team team@swoole.com
Version => 4.7.1
Built => Oct 9 2021 00:21:35
coroutine => enabled with boost asm context
epoll => enabled
eventfd => enabled
signalfd => enabled
spinlock => enabled
rwlock => enabled
mutex_timedlock => enabled
pthread_barrier => enabled
async_redis => enabled

Directive => Local Value => Master Value
swoole.enable_coroutine => On => On
swoole.enable_library => On => On
swoole.enable_preemptive_scheduler => Off => Off
swoole.display_errors => On => On
swoole.use_shortname => On => On
swoole.unixsock_buffer_size => 8388608 => 8388608

5. What is your machine environment used (show your uname -a & php -v & gcc -v) ?

Linux 556ec368b5d0 5.10.47-linuxkit #1 SMP Sat Jul 3 21:51:47 UTC 2021 x86_64 Linux

PHP 8.0.11 (cli) (built: Sep 23 2021 20:15:09) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.11, Copyright (c) Zend Technologies

[ValiDrv]

Your trying to put the connection back twice... Once on error and once in your finally block.

[intellent]

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.

[ValiDrv]

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)

[twose]

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.

[intellent]

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.

[twose]

The reason why it was designed as fatal error is that you have to fix your code, it should not be caught.

[yespire]

@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:

1. $pool->put(); // obviously bad
2. $pool->put($pdo); // after this, swoole process dies
3. $pool->put(false); // obviously bad, FALSE is assigned to another process, then die
4. $pool->put(NULL); // no problem, no error

Here are 3 ideas at application level:

1. create a wrapper class for pool, implement a idempotent put() method by spl_object_id($conn)
2. keep pool->get() and pool->put() as close as possible, "pool" concept is for limited resources, therefore good idea to checkout/use/return immediately
3. set $conn = null after $pool->put($conn)

Can someone please share some insight as why put(conn) is not idempotent ?

[intellent]

@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.

[ValiDrv]

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]);

[yespire]

@ValiDrv

It doesn't make sense not to have an error/exception if you put back the same connection multiple times

• seems that you and OP agree

@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:

1. $pool->put($same_conn) doesn't throw error/exception, it does not crash process itself either
   the problem arise after pool assign the $same_conn to another subroutine. You can expect PDOException from PDOStatement execute(), It's reasonable to enclose the whole logic block in the same try-catch, so if you just want to watch out for exception, there is no need to enhance put() method

try {
    $conn = pool->get();
    ...
    $stmt = $conn->prepare_statement()
    $stmt->execute()  // exception
    ...
    pool->put($conn);
} catch (\Throwable $e) {
     // notify dev
}

2. 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

3. 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

4. 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)