WeakMap prevents garbage collection

[nicolas-grekas]

Description

I have a situation where I create an object graph similar to this one:

class Canary extends stdClass
{
    public function __construct(public string $name)
    {
    }

    function __destruct()
    {
        echo $this->name."\n";
    }
}

$container = new Canary('container');
$canary = new Canary('canary');
$container->canary = $canary;

$map = new \WeakMap();
$map[$canary] = $container;

echo 1;
unset($container, $canary);
gc_collect_cycles();
echo 2;

Resulted in this output:

12
container
canary

But I expected this output instead:

1container
canary
2

Commenting the $map[$canary] = $container; line generates the expected output.

A simpler reproducer is this one:

$canary = new Canary('canary');

$map = new \WeakMap();
$map[$canary] = $canary;

echo 1;
unset($canary);
gc_collect_cycles();
echo 2;

Which resulted in this output:

12
canary

But I expected this output instead:

1canary
2

PHP Version

PHP >= 8.0

Operating System

No response

[nicolas-grekas]

I guess there is little to no way around, isn't it? That makes WeakMap a bit of a foot-gun when storing arbitrarily complex variables on the right side, where such loops can happen and be unnoticed. I realize that this makes a good use case for dynamic properties BTW: they're superior to weak-maps in this regard since they would be collected in the case at a glance, whatever the right side.

[iluuu1994]

AFAIK you shouldn't use the same object as a value, as the value itself is still refcounted.

https://www.php.net/manual/en/class.weakmap.php

A WeakMap is map (or dictionary) that accepts objects as keys. However, unlike the otherwise similar SplObjectStorage, an object in a key of WeakMap does not contribute toward the object's reference count.

$map[$canary] = 'foo';

https://3v4l.org/Ic0YC

gives you the expected result.

Also note that no garbage collection is involved here, which is only relevant when there are reference cycles.

[nicolas-grekas]

Also note that no garbage collection is involved here, which is only relevant when there are reference cycles.

At first I didn't understand this sentence, and then I realized: isn't this precisely the fix that could be needed? Making garbage collection also consider weak maps?

[iluuu1994]

@nicolas-grekas PHP implements reference counting which takes care of everything except reference cycles. All the garbage collector in PHP does is try to find these cycles. Your example doesn't involve cycles, so the garbage collector can't help. The reason your example leaks is because WeakMap still increases the refcount for the values. Otherwise, this would not work properly:

$map[$a] = new B();
// B is immediately released, even if $a hasn't

[iluuu1994]

@nicolas-grekas Can you clarify how you're currently using WeakMap? I'm assuming it should be possible to just use a different dummy-value instead.

[KapitanOczywisty]

WeakReference could be used instead.

@iluuu1994 JS in similar case will remove such object, however PHP has iteration on the WeakMap object - which JS doesn't have. Though I feel if something is only accessible through WeakMap should be removed, even if it's not technically a cycle.

[damianwadley]

If it hasn't been pointed out explicitly yet, the "weak"-ness of WeakMap applies to the keys but not the values.

So:

1. The container depends on the canary, therefore
2. The canary cannot be dropped until after the container is dropped, therefore
3. A WeakMap of canary => container, where the canary is weakly-referenced and the container is not, cannot be cleaned up by unsetting their original variables (because the container is still being referenced by the map itself).

If the association was $container => $canary instead, that could be cleaned up because (1) the container is weakly-referenced and so drops to refcount 0 when the original variable is unset, at which point (2) the canary is no longer used and so it can also be cleaned up.
https://3v4l.org/VN2qr

WeakReference indeed solves this because (1) the container no longer depends on the canary, therefore (2) either can be dropped at any time, therefore (3) a canary => container association can be cleaned up.
https://3v4l.org/QFobe

[iluuu1994]

Apologies, I only read the second, condensed example. But as @damianwadley explained, the container is kept alive by the weak map, and $canary by the property of the container. So there's no bug here in either examples. Don't hesitate to comment if anything is still unclear and I'll reopen the issue.

[nicolas-grekas]

I totally get that the current behavior has rational technical ground and that the behavior can be explained.
So let's consider this as a request for improvement instead of a bug fix, because the current behavior is not useful to userland.

JS in similar case will remove such object, however PHP has iteration on the WeakMap object - which JS doesn't have. Though I feel if something is only accessible through WeakMap should be removed, even if it's not technically a cycle.

100% this.

In https://wiki.php.net/rfc/deprecate_dynamic_properties, @nikic writes:

In some cases it is desirable to associate information with objects that you do not own. Previously, it was possible to add a dynamic property for this purpose. Instead, a WeakMap should be used to store the information in a non-intrusive way:

And yet, this is not fully accurate, because weakmaps have this behavior that make them a footgun:

AFAIK you shouldn't use the same object as a value

This is the footgun: I simplified the example up to the maximum, but in real applications, the object graph can be arbitrarily complex, and it's not always possible to ensure that a variable won't be self-referenced by a weakmap. (In my case, there are multiple nesting level involving a container, variables bound to closures, etc.)

Could you please reopen and consider if this would make sense as a feature request?