Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(transaction): transaction is never released #50

Closed
wants to merge 1 commit into from

Conversation

azjezz
Copy link

@azjezz azjezz commented Jan 26, 2022

Given the following example:

$config = Postgres\ConnectionConfig::fromString('host=127.0.0.1 port=32770 user=main password=main');
$link = Postgres\connect($config);
$trans = $link->beginTransaction();
$trans->query('DROP TABLE IF EXISTS test_users');
$trans->query('CREATE TABLE IF NOT EXISTS test_users (
        id SERIAL PRIMARY KEY,
        username VARCHAR(30) NOT NULL,
        email VARCHAR(1024) NOT NULL,
        password VARCHAR(1024) NOT NULL,
        status INT NOT NULL
    )');
$trans->commit();

$link->execute('INSERT INTO test_users (username, email, password, status) VALUES (?,?,?,?)', ['azjezz', '[email protected]', '123456789', 1]);
$users = $link->execute('SELECT * FROM test_users');

var_dump(\iterator_to_array($users));

Prior to this change, i encounter the following error:

> php users.php
PHP Fatal error:  Uncaught Error: Event loop terminated without resuming the current suspension in /home/azjezz/Projects/neutomic/neu/vendor/revolt/event-loop/src/EventLoop/Internal/DriverSuspension.php:86
Stack trace:
#0 /home/azjezz/Projects/neutomic/neu/vendor/amphp/amp/src/Future.php(244): Revolt\EventLoop\Internal\DriverSuspension->suspend()
#1 /home/azjezz/Projects/neutomic/neu/vendor/amphp/postgres/src/Connection.php(75): Amp\Future->await()
#2 /home/azjezz/Projects/neutomic/neu/vendor/amphp/postgres/src/Connection.php(113): Amp\Postgres\Connection->awaitPending()
#3 /home/azjezz/Projects/neutomic/neu/example/users.php(25): Amp\Postgres\Connection->execute('INSERT INTO tes...', Array)
#4 {main}
  thrown in /home/azjezz/Projects/neutomic/neu/vendor/revolt/event-loop/src/EventLoop/Internal/DriverSuspension.php on line 86

Fatal error: Uncaught Error: Event loop terminated without resuming the current suspension in /home/azjezz/Projects/neutomic/neu/vendor/revolt/event-loop/src/EventLoop/Internal/DriverSuspension.php:86
Stack trace:
#0 /home/azjezz/Projects/neutomic/neu/vendor/amphp/amp/src/Future.php(244): Revolt\EventLoop\Internal\DriverSuspension->suspend()
#1 /home/azjezz/Projects/neutomic/neu/vendor/amphp/postgres/src/Connection.php(75): Amp\Future->await()
#2 /home/azjezz/Projects/neutomic/neu/vendor/amphp/postgres/src/Connection.php(113): Amp\Postgres\Connection->awaitPending()
#3 /home/azjezz/Projects/neutomic/neu/example/users.php(25): Amp\Postgres\Connection->execute('INSERT INTO tes...', Array)
#4 {main}
  thrown in /home/azjezz/Projects/neutomic/neu/vendor/revolt/event-loop/src/EventLoop/Internal/DriverSuspension.php on line 86

Now, this works as expected:

> php users.php
array(1) {
  [0]=>
  array(5) {
    ["id"]=>
    int(1)
    ["username"]=>
    string(6) "azjezz"
    ["email"]=>
    string(21) "[email protected]"
    ["password"]=>
    string(9) "123456789"
    ["status"]=>
    int(1)
  }
}

( note: the example above uses the v2 branch, but i'm assuming the issue is also present in v1 )

@azjezz
Copy link
Author

azjezz commented Jan 26, 2022

Hm 🤔 commit doesn't decrement the refCount, is it supposed to?

@azjezz
Copy link
Author

azjezz commented Jan 26, 2022

decrementing the refCount on commit/rollback also fixes this problem, so I'm not sure which one is the correct fix.

@trowski
Copy link
Member

trowski commented Jan 27, 2022

Yes, it does in v1, seems that was overlooked when upgrading to v2.

@azjezz azjezz force-pushed the patch-1 branch 2 times, most recently from 08ef3fd to 31f49f7 Compare January 27, 2022 01:02
@azjezz azjezz changed the base branch from master to v2 January 27, 2022 01:02
@azjezz
Copy link
Author

azjezz commented Jan 27, 2022

tests are passing locally 🎉

Comment on lines +570 to +585
$result = $transaction->query("DELETE FROM test");
unset($result);
$transaction->rollback();

try {
$transaction->execute("SELECT * FROM test");
$this->fail('Query should fail after transaction commit');
} catch (TransactionError $exception) {
// Exception expected.
}

$this->assertTrue($this->link->isAlive());

$result = $this->link->execute('SELECT * FROM test WHERE domain = ?', [$data[0]]);
self::assertSame(1, $result->getRowCount());
unset($result);
Copy link
Author

Choose a reason for hiding this comment

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

this still feels buggy to me, even tho it's consistent with v1.

If we remove unset($result) in line 571, we still get the same error, as in line 583, the query is executed before $result is destructed, so the transaction is not yet released.

wdyt @trowski?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants