amphp / postgres

Async Postgres client for PHP based on Amp.
MIT License
97 stars 20 forks source link

[V2] Pending transaction rollbackTo in PHPUnit #55

Closed PNixx closed 1 year ago

PNixx commented 2 years ago

Example file:

<?php

namespace Amp\Postgres\Test;

use Amp\PHPUnit\AsyncTestCase;
use Amp\Postgres\PostgresConfig;
use Amp\Postgres\PostgresConnectionPool;
use Amp\Postgres\PostgresTransaction;
use function Amp\async;
use function Amp\Future\await;

class PgSqlAwaitTest extends AsyncTestCase
{
    protected PostgresConnectionPool $connection;
    protected PostgresTransaction $transaction;

    protected function setUp(): void
    {
        parent::setUp();
        $this->connection = new PostgresConnectionPool(PostgresConfig::fromString("host=localhost user=postgres password=postgres"));
        $this->transaction = $this->connection->beginTransaction();
        $this->transaction->createSavepoint('phpunit_savepoint');
    }

    protected function tearDown(): void
    {
        $this->transaction->rollbackTo('phpunit_savepoint');
        $this->transaction->rollback();
        $this->connection->close();
    }

    public function testAwait(): void
    {
        $this->expectExceptionMessage('First');
        await([
            async(function () {
                $this->transaction->query('SELECT 1');
                throw new \Exception('First');
            }),
        ]);
    }

    public function testAwaitMultiple(): void
    {
        $this->expectExceptionMessage('First');
        await([
            async(function () {
                $this->transaction->query('SELECT 1');
                throw new \Exception('First');
            }),
            async(function () {
                $this->transaction->query('SELECT 2');
            }),
        ]);
    }
}

testAwait - completes successfully, but testAwaitMultiple hangs constantly waiting on the line $this->transaction->rollbackTo('phpunit_savepoint'); in tearDown method.

I think need modify await function to:

function await(iterable $futures, ?Cancellation $cancellation = null): array
{
    $values = [];
    $errors = [];

    // Future::iterate() to throw the first error based on completion order instead of argument order
    foreach (Future::iterate($futures, $cancellation) as $index => $future) {
        try {
            $values[$index] = $future->await();
        } catch (\Throwable $throwable) {
            $errors[$index] = $throwable;
        }
    }
    foreach ($errors as $error) {
        throw $error;
    }

    /** @var array<Tk, Tv> */
    return $values;
}
kelunik commented 2 years ago

await short-circuits on exceptions by design, so the proposed change isn't going to be merged as is. I'm not sure why it hangs, but I'm also not sure how concurrency safe these transaction objects are.

trowski commented 1 year ago

I just tested with the latest commit from the 2.x branch and did not observe any hanging behavior. I'm not certain, but the reason may be linked to https://github.com/amphp/amp/pull/395, which would allow the GC of the result and transaction objects, preventing the hang. For some silly reason we didn't tag a version of amphp/amp with that commit until November, so it makes sense you'd have still seen the old behavior in October.

I'm closing this issue then as I believe this has been fixed. Thanks for reporting! 👍