php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #75190 Impossible to close PDO connection when extending PdoStatement using $this
Submitted: 2017-09-11 15:58 UTC Modified: 2017-10-06 15:14 UTC
From: ian at oshaughnessy dot cc Assigned:
Status: Not a bug Package: PDO related
PHP Version: 7.0.23 OS: Debian 9
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: ian at oshaughnessy dot cc
New email:
PHP Version: OS:

 

 [2017-09-11 15:58 UTC] ian at oshaughnessy dot cc
Description:
------------
The only way to close a PDO connection is to set the resource variable to null (thereby destroying the PDO object); however, when extending the PdoStatement class, it's required that $this is passed as an option to the PDO object for Pdo::ATTR_STATEMENT_CLASS.

In doing this, a reference to the PDO object is created, making it impossible to destroy the PDO object since a lingering reference continues to exist after $pdo is set to null.

The only way to fix this issue is to use Pdo::setAttribute() after the PDO parent constructor has been run.

Test script:
---------------
<?php

echo "Test 1 (PdoTestBroken)\n";
echo "new PdoTestBroken\n";
$pdo1 = new PdoTestBroken($dsn, $username, $password, []);
echo "Destroy \$pdo\n";
$pdo1 = null;
echo "----------------------------\n\n";

echo "Test 2 (PdoTestWorks)\n";
echo "new PdoTestWorks\n";
$pdo2 = new PdoTestWorks($dsn, $username, $password, []);
echo "Destroy \$pdo\n";
$pdo2 = null;
echo "----------------------------\n\n";

class PdoTestBroken extends Pdo {

	public function __construct($dsn, $username, $passwd, $options) {
		echo "PdoTest1::__construct()\n";
		$options[self::ATTR_STATEMENT_CLASS] = [ 'PdoStatementTest', [ $this, ] ];
		parent::__construct($dsn, $username, $passwd, $options);
	}

	public function __destruct() {
		echo "PdoTest1::__destruct()\n";
	}
}

class PdoTestWorks extends Pdo {

	public function __construct($dsn, $username, $passwd, $options) {
		echo "PdoTest2::__construct()\n";
		$self = parent::__construct($dsn, $username, $passwd, $options);
		$this->setAttribute(PDO::ATTR_STATEMENT_CLASS, [ 'PdoStatementTest', [ $self, ] ]);
	}

	public function __destruct() {
		echo "PdoTest2::__destruct()\n";
	}
}

class PdoStatementTest extends PdoStatement {

}

Expected result:
----------------
The connection to close.


Actual result:
--------------
The connection does not close due to hidden reference variable.

Script Results:

Test 1 (PdoTestBroken)
new PdoTestBroken
PdoTest1::__construct()
Destroy $pdo
----------------------------

Test 2 (PdoTestWorks)
new PdoTestWorks
PdoTest2::__construct()
Destroy $pdo
PdoTest2::__destruct()
----------------------------

PdoTest1::__destruct()

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-10-05 20:42 UTC] adambaratz@php.net
-Status: Open +Status: Not a bug
 [2017-10-05 20:42 UTC] adambaratz@php.net
You could pass an empty array for the ctor args. It's not strictly required that you pass a reference to the PDO object. I'm not sure this is a PDO issue, so much as a side effect of PHP's reference counting.

For the sake of clarity, I'd recommend avoiding doing what you're doing here. Basically, because it's easy to create situations like this one. If you want the statement to have a reference to the connection, I'd recommend composition over inheritance. That is, creating your own PDO and PDOStatement classes that wrap the built-ins.

There is the separate point that it's awkward that you can't close a connection without destructing the PDO object. If that's an issue you want to address, please file a feature request ticket. Or better still, put together an RFC to hash out the details with the internals community.
 [2017-10-05 21:34 UTC] nikic@php.net
I remember fixing this issue... and indeed, the pdo_dbh get_gc handler returns def_stmt_ctor_args: https://github.com/php/php-src/blob/master/ext/pdo/pdo_dbh.c#L1382

As such this should not be an actual leak (like it was on PHP 5), but the destruction is delayed until the next cycle GC. Explicitly calling gc_collect_cycles() may resolve the problem.
 [2017-10-05 23:20 UTC] ian at oshaughnessy dot cc
I guess my only issue with this being flagged as 'not a bug' is that you can cause a memory leak by writing valid PHP code, which seems pretty buggy.

Side note, extending the Pdo library is the only option in our case, since composition means we cannot use any 3rd party libs, since no external code base is going to want access to an "IansPdoClass" wrapper object, they want the Pdo object.
 [2017-10-06 15:14 UTC] adambaratz@php.net
Is it a true memory leak? As in, after the process finishes, the memory's not freed?

I'm pushing on either a wrapper class or adding a close connection method because it feels like a risky design practice to create this kind of reference and expect garbage collection to trigger in a particular way. Perhaps you could create a separate class for passing whatever information you need from the connection to the statement? Or you could create some sort of registry that coordinates between connections and statements?
 [2020-11-11 13:57 UTC] niklas dot brunberg at ateles dot se
We have documented more on this issue here: https://github.com/doctrine/dbal/issues/3047
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Apr 25 09:01:29 2024 UTC