Description:
------------
Hey! First of all, thanks for the Weakref extension. I'm using it extensively in my ORM (thecodingmachine/tdbm) and it is definitely a life-saver.
I've recently encountered a strange problem and I want to share it here.
The Weakref object has 2 methods to check for an object: `valid` and `get`.
So in most of the code using this extension, you will see:
```
if ($weakref->valid()) {
$myobj = $weakref->get();
$myobj->doSomeStuff();
}
```
This code is actually buggy. Indeed, you never know when the garbage collector might be triggered. If the garbage collector is triggered between `$weakref->valid()` and `$weakref->get()`, then `$weakref->get() === null`.
This actually happened to me today (for the first time after 5 years of use of Weakref):
https://github.com/thecodingmachine/tdbm/pull/80#issuecomment-385902994
If I disable the garbage collector, everything runs fine:
```
gc_disable();
if ($weakref->valid()) {
$myobj = $weakref->get();
$myobj->doSomeStuff();
}
gc_enable();
```
When I think about it, I come to realize that the `valid()` method is really deceptive (because you cannot trust it to perform a "get" request afterwards.
The only sane way to write this code is:
```
$myobj = $weakref->get();
if ($myobj !== null) {
$myobj->doSomeStuff();
}
```
I'm not sure what is the good way to solve this but I think we should either:
- deprecate the "valid()" method
- or write a huge warning in the "valid()" method documentation

History

Hello,
you are right that there is some race-condition to valid() followed by get() that makes it somewhat dangerous to trust as is.
Have you tried to acquire() the weak ref instead of checking valid()? If acquire is successful you should be safe to get() it until you release().
Best
Instead of deprecating valid() or disabling the GC in the middle of the critical section, how about you simply acquire() the weakref? If this returns true, you know that it is safe to get, which you can then release() later.

[2018-05-02 13:32 UTC] d dot negrier at thecodingmachine dot com

-Status: Feedback+Status: Assigned

[2018-05-02 13:32 UTC] d dot negrier at thecodingmachine dot com

Hey Etienne,
Thanks a lot for the quick reply.
You are perfectly right that using "acquire" makes a lot of sense and solves the race condition:
```
if ($weakref->acquire()) {
$myobj = $weakref->get();
$myobj->doSomeStuff();
$weakref->release();
}
```
This is great.
But you are kind of proving my point: the "valid" method is useless in this case since it is indeed replaced by acquire.
There are very few cases where "valid()" is useful and it is too tempting to use it. A proper warning in the doc might save some people.

Hello,
I'm not sure what you mean?
If at the time of the get() call the weakref is still valid, it will increase its refcount as it returns an actual zval of it. From that point on and until it is destroyed, the weakref reference will not be collectible as it has a >=1 refcount.

Ah, thanks! So ::acquire()/::release() is not strictly
necessary, so that the example above could also be written as:
if (($myobj = $weakref->get())) {
$myobj->doSomeStuff();
}

[2018-05-03 15:10 UTC] d dot negrier at thecodingmachine dot com

@cmb: absolutely.
Actually, if you want to fetch an object from a WeakRef, the correct way to go is:
<?php
if ($myobj = $weakref->get()) {
$myobj->doSomeStuff();
}
?>
My whole point is that it is too easy for a beginner using this function to write:
<?php
if ($weakref->valid()) { // returns true
// If you are unlucky, the garbage collector might be triggered here
$myobj = $weakref->get(); // will return null
$myobj->doSomeStuff(); // boom
}
?>
This is wrong because the garbage collector may be triggered between first and second line.
I added a user contributed note on the "WeakRef::valid()" method page. It should be published soon. http://php.net/manual/en/weakref.valid.php