-
Notifications
You must be signed in to change notification settings - Fork 209
Fix foreach after the garbage collector is invoked #1850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixed up everything!
The codebase was successfully formatted using scripts/clang-format.sh locally with clang-format 20, not sure why is it complaining here; can't access the evergreen tests so I can't see what's wrong there
I can take a look at the clang-format issue.
It looks like there is a memory leak in some tests:
[2025年07月28日 09:44:20.753] FAILURE: (FAILED)
[2025年07月28日 09:44:20.753] --
[2025年07月28日 09:44:20.753] same serverConnectionId as last commandStarted: yes
[2025年07月28日 09:44:20.753] int(%d)
[2025年07月28日 09:44:20.753] OK: Got MongoDB\Driver\Exception\CommandException
[2025年07月28日 09:44:20.753] 007+ [Mon Jul 28 07:43:20 2025] Script: '/data/mci/c0332320a42282d79356368a6fe838a7/src/tests/apm/commandFailedEvent-getServerConnectionId-001.php'
[2025年07月28日 09:44:20.753] 008+ /data/mci/5683cf19da3859207a5bbd4900e33449/src/src/MongoDB/Manager.c(253) : Freeing 0x00007f3fcc07c180 (56 bytes), script=/data/mci/c0332320a42282d79356368a6fe838a7/src/tests/apm/commandFailedEvent-getServerConnectionId-001.php
[2025年07月28日 09:44:20.753] 009+ === Total 1 memory leaks detected ===
Our tests on GHA don't use debug versions of PHP, so these leaks are not reported there. The offending line in Manager.c allocates the intern->subscribers hash table:
mongo-php-driver/src/MongoDB/Manager.c
Line 253 in ae08bbc
I can see that in a number of classes, you have replaced the calls to
zend_hash_destroy followed by FREE_HASHTABLE with a single call to zend_hash_release, but left just a single call to zend_hash_destroy in Manager.c.
If you are testing with a debug version of PHP locally, I recommend calling ./configure with the --enable-mongodb-developer-flags switch to enable memory leak detection and other development checks.
I simply compiled both php and the extension with ASAN/LSAN, which is even better than PHP's builtin leak detection, but the issue is that there are many unfixed leaks in PHP itself when dealing with fatal errors (such as the invalid class extension tests, etc), so I just focused on finding leaks in the GC/cycle tests, that's why I missed this one :)
Re: the usage of zend_hash_release, I'd actually also replace the remaining places where zend_hash_destroy/HASH_RELEASE is used as it considers the refcount/immutability (not present in this case, but who knows in the future) and automatically frees the hashtable struct as well, though probably it would be better to do this in a separate PR...
@danog the windows failure is unrelated, and the first impression from Evergreen is that the memory leak is no more. As for the remaining zend_hash_destroy usage, I have no objection to changing those as well, but a separate PR might indeed be the way to go for this. Please be aware that @jmikola is at Laracon these days, so it might be a few days until we can review and merge this PR.
Uh oh!
There was an error while loading. Please reload this page.
Fixes #1776, https://jira.mongodb.org/browse/PHPC-2505, and the equivalent bug triggered when assigning dynamic properties and then unsetting them again.
The underlying issue in both cases is the invocation of
zend_std_get_properties, which populates thepropertieshashtable in the zend object (not in the interned mongo one).This, in turn, causes the iteration logic to completely skip iteration due to the
if (zend_hash_num_elements(properties) == 0) {check in theZEND_FE_RESET_R_SPEC_CV_HANDLERThe fix adds custom handlers for
get_property,write_property,unset_property,has_property,get_property_ptr_ptrwhich use a new interned php_properties hashtable, instead of the zend hashtable property.The
zend_gchandler is also modified to invokeget_propertiesinstead ofzend_std_get_properties.The overall implementation mostly works as expected, the only issue is some strange refcounting behavior within the garbage collector on the hashtable returned by get_gc, which sometimes increments the refcount without decrementing it again, leading to a leak of the interned
propertieshashtable (detectable using ASAN); I have asked clarifications regarding that, marking the PR as draft in the meantime.There's also a leftover issue with foreach loops not exiting for whatever reason, still looking into that.