PHPC-2505: Fix foreach after GC invocation#1940
Conversation
As previously reported, iterating objects with custom get_properties handlers (i.e. serializable classes) was inconsistent depending on whether or not garbage collection had occurred. Remove unconditional invocation of zend_std_get_properties from the extension's standard get_gc handler. This prevents population of properties on the internal zend_object. Serializable classes that define a custom get_properties handler (also used for var_export) will now define a separate php_properties HashTable. Additional property handlers (including get_gc) will operate exclusively on php_properties. Long-term, get_properties may still be replaced with get_properties_for (see: PHPC-1622). Doing so may allow us to eliminate get_properties in most cases. This commit also utilizes zend_hash_release() from PHP 8.0 to free HashTables. Additional instances of zend_hash_destroy() can be revised in PHPC-2682.
jmikola
requested review from
alcaeus and
paulinevos
and removed request for
a team and
paulinevos
alcaeus
left a comment
alcaeus
left a comment
There was a problem hiding this comment.
I left some comments in the code. Besides that, I'm wondering whether this is in fact the right approach here. Part of the problem seems to be that we're treating our "debug" properties as actual properties, which requires us to duplicate this internal handling here. However, it sounds like implementing get_properties_for would (at least partially) resolve this. My worry is that with this implementation, we're adding custom dynamic property access for all of our classes, which will block us from effectively adding public properties for things (e.g. for Server instances) in the future. The implementation of write_property also bypasses the default behaviour of newer PHP versions, which emit a deprecation notice about defining dynamic properties on objects that aren't explicitly marked as supporting them:
php > $rp = new MongoDB\Driver\ReadPreference('primary');
php > $rp->foo = 'bar';
Deprecated: Creation of dynamic property MongoDB\Driver\ReadPreference::$foo is deprecated in php shell code on line 1
Call Stack:
13.9274 426584 1. {main}() php shell code:0
In my opinion, we should not introduce this custom property behaviour, but only focus on ensuring that we don't go out of scope if someone holds a reference to our properties, e.g. for iteration.
| if (intern->properties) { | ||
| zend_hash_destroy(intern->properties); | ||
| FREE_HASHTABLE(intern->properties); | ||
| zend_hash_release(intern->properties); | ||
| } | ||
|
|
||
| if (intern->php_properties) { | ||
| zend_hash_release(intern->php_properties); | ||
| } |
There was a problem hiding this comment.
Refactoring suggestion: we could extract this to a PHONGO_RELEASE_INTERN_PROPERTIES macro as this code is the same in all classes. Feel free to defer this to a follow-up ticket.
There was a problem hiding this comment.
I did originally have a PHONGO_FREE_PROPS macro but removed it before finalizing the commit because I felt it made the free_object handler a bit less clear. And I didn't like it was selectively used only in classes that defined both properties and php_properties on their internal struct. Happy to add that back, though, as it would remove duplication.
| { \ | ||
| 0 \ | ||
| } \ | ||
| { 0 } \ |
There was a problem hiding this comment.
Note that this diff is causing the coding standards check to break. I'm not sure what the cause of this is, but it would seem that the version of clang-format installed on MacOS produces different output of the one on GitHub Actions.
There was a problem hiding this comment.
Very likely. I can revert this, as the file itself wasn't actually relevant to the PR and I debated including this.
| --SKIPIF-- | ||
| <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?> | ||
| <?php skip_if_php_version('>=', '8.1.99'); ?> |
There was a problem hiding this comment.
I don't remember the reason why this test only applied on PHP 8.2+, but what about these changes makes it applicable to all versions?
3 participants
https://jira.mongodb.org/browse/PHPC-2505
Supersedes #1850 and fixes #1776
@danog: I ended up rebasing your original PR and making a number of revisions, but preserved your authorship. Thanks for your initial work on this, and all of the research on the php-src repo.
As previously reported, iterating objects with custom get_properties handlers (i.e. serializable classes) was inconsistent depending on whether or not garbage collection had occurred.
Remove unconditional invocation of zend_std_get_properties from the extension's standard get_gc handler. This prevents population of properties on the internal zend_object. Serializable classes that define a custom get_properties handler (also used for var_export) will now define a separate php_properties HashTable. Additional property handlers (including get_gc) will operate exclusively on php_properties.
Long-term, get_properties may still be replaced with get_properties_for (see: PHPC-1622). Doing so may allow us to eliminate get_properties in most cases.
This commit also utilizes zend_hash_release() from PHP 8.0 to free HashTables. Additional instances of zend_hash_destroy() can be revised in PHPC-2682.