Skip to content

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

Open
wants to merge 45 commits into
base: v2.x
Choose a base branch
from

Conversation

danog
Copy link

@danog danog commented Jul 24, 2025

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 the properties hashtable 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 the ZEND_FE_RESET_R_SPEC_CV_HANDLER

The fix adds custom handlers for get_property, write_property, unset_property, has_property, get_property_ptr_ptr which use a new interned php_properties hashtable, instead of the zend hashtable property.

The zend_gc handler is also modified to invoke get_properties instead of zend_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 properties hashtable (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.

@danog danog marked this pull request as ready for review July 25, 2025 10:21
@danog danog requested a review from a team as a code owner July 25, 2025 10:21
@danog danog requested review from alcaeus and removed request for a team July 25, 2025 10:21
@danog
Copy link
Author

danog commented Jul 25, 2025

Fixed up everything!

@alcaeus alcaeus requested review from jmikola and removed request for alcaeus July 28, 2025 07:38
@danog
Copy link
Author

danog commented Jul 28, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

foreach over ObjectId and Garbage Collection
1 participant