Summary
I'm working with a legacy codebase. Refactoring a particular class leads to lockup (website never loading, tests getting stuck). Attempts to track a problematic piece of code with Xdebug trigger heisenbug behavior. Dissecting the changes leads to a property assignment in constructor that triggers the hang.
Many details below are probably irrelevant, but due to the weird nature of this problem I don't know what's actually relevant and what's not, so I'm trying to not omit anything important. So please bear with me.
Original situation
It's a classic website, with PHP rendering whole pages. The codebase is old, doesn't use any framework and doesn't follow good practices or patterns. We're aware of major problems with this app, but it's in the process of being replaced and we're doing only minimum necessary maintenance until we're done with it.
The particular class I'm trying to refactor is a wannabe singleton wrapper for a DBAL connection. Here's a small part of the original class: (original is 240 lines long)
final class MysqliConnection implements Connection
{
private static $connection;
private function __construct(DbalConnection $connection)
{
self::$connection = $connection;
}
public static function fromDefaults(bool $forceReconnect = false): self
{
if (self::$connection && !$forceReconnect) {
return new self(self::$connection);
}
return self::fromDefaultConfig();
}
// fromDefaultConfig() omitted for brevity
public function fetch(string $sql, ...$params): array
{
$stmt = self::tryCall(self::$connection, static function (DbalConnection $connection) use ($sql, $params) {
// snip...
});
// snip...
}
// a bunch of instance methods omitted for brevity
}
In a nutshell, all instance methods operate on a static property $connection
and there is no instance state at all. The constructor serves no purpose but to assign the static property. This assignment could be moved to fromDefaults()
. It's an all-static class in a singleton disguise.
Problem
For reasons, I want to refactor this class to a classic singleton, where the class is actually instantiated with the connection it's supposed to work on. My plan:
- Add a private property to store the connection
- Change all instance methods to work with that property rather than the static one
- Move the static property assignment to
fromDefaults()
- Assign the new property in the constructor
(I am aware that this code isn't exactly equivalent because fromDefaults(true)
doesn't affect existing connections anymore, but I'm okay with that.)
To my surprise, as soon as I change the constructor to look like this:
private function __construct(DbalConnection $connection)
{
$this->instanceConnection = $connection;
}
... PHPUnit starts getting stuck on a not clearly related test and the website is stuck in infinitely loading state in the browser. The only other change I made is moving self::$connection = $connection;
to fromDefaults()
. $this->instanceConnection
isn't used anywhere yet, it's just assigned in the constructor never to be touched again.
Here's (redacted) PHPUnit's output when called with --testdox --debug
for extra details:
(... lots of successful tests up until this point ...)
Test 'AppTestsIntegrationUploadTest::testUploadImage' started
Test 'AppTestsIntegrationUploadTest::testUploadImage' ended
? Upload image 0 ms
┐
├ Test skipped because of an error in hook method
│
? /platform-root/app/foo/vendor/phpunit/phpunit/phpunit:61
┴
Test 'AppTestsIntegrationUploadTest::testUploadOtherImage' started
Test 'AppTestsIntegrationUploadTest::testUploadOtherImage' ended
? Upload other image 0 ms
┐
├ Test skipped because of an error in hook method
│
? /platform-root/app/foo/vendor/phpunit/phpunit/phpunit:61
┴
Test 'AppTestsIntegrationApiFilterTest::testGetFilterByZoneId' started
(... never progresses past this point ...)
(These errors in PHPUnit are expected and not the cause of the problem - they appear with the working version too.)
Further experiments show that:
- It doesn't matter if
$this->instanceConnection
is declared in class or not - Using different property names doesn't fix the problem
- Assigning other values to this property works (
$this->instanceConnection = null;
etc.) - Assigning
$connection
to a local variable works ($test = $connection;
) - But assigning property through an intermediate variable blocks again (
$test = $connection; $this->instanceConnection = $test;
) - Disabling
strict_types
doesn't fix the problem - Disabling Xdebug in PHP config doesn't fix the problem
Heisenbug behavior with Xdebug
Before finding this particular line that causes the issue, I've been trying to pinpoint it with Xdebug, but it has produced weird results.
If I place a breakpoint at the very beginning of the index.php
, it works fine, but a breakpoint roughly in the middle of that file is never reached - the website apparently blocks above it. However, stepping through the code line by line lets me reach that breakpoint with high success rate (not 100% reliably though, there's some randomness to it). Basically watching the code with a debugger makes the problem go away. Larger blobs of code are still causing lockups, but stepping into them and progressing a few lines at a time makes them work again. Looks like a plain old multithreaded deadlock to me, but I don't know how that would be possible in PHP.
The platform
# php -v
PHP 7.4.11 (cli) (built: Oct 6 2020 10:34:39) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
with Zend OPcache v7.4.11, Copyright (c), by Zend Technologies
with Xdebug v2.9.8, Copyright (c) 2002-2020, by Derick Rethans
Running in Docker - custom image based on Debian Bullseye. The issue is reproduced on my Linux machine and in GitLab pipelines.
host$ uname -r
5.4.0-60-generic