Release a transit shell's C state through the new transfer kind - #232
Merged
Conversation
thread_release_transferred_object walked the shell's properties, dropped the class name and pefreed the allocation. Anything a transfer_obj handler had put in the shell's C prefix was never reached, because free_obj does not run for a shell — the TrueAsync server leaked a topic-hub reference per transferred room this way, 21,608 bytes of hub plus the persistent topic string. The walk now resolves the shell's class by name and calls its handler with ZEND_OBJECT_TRANSFER_RELEASE before the generic cleanup. No autoload: a release runs outside a request, and a class that is not loaded owns nothing here. The three handlers in this extension refuse the new kind explicitly, because each branches on TRANSFER and treats everything else as LOAD — reaching that branch with a NULL default_fn would crash. Closure's snapshot is already freed by the walk itself; the channel reference and the future's shared state move under protocols of their own, and reworking those is a separate change.
Contributor
Author
|
CI is red on the build step for the stated dependency, not for a defect here: the runners build php-src from the It turns green once true-async/php-src#20 lands and this branch is re-run. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Needs true-async/php-src#20, which adds the kind this dispatches.
thread_release_transferred_objectwalked the shell's properties, dropped the class name andpefreed the allocation. Anything atransfer_objhandler had put in the shell's C prefix was never reached, becausefree_objdoes not run for a shell. The TrueAsync server leaked a topic-hub reference for every room transferred into a pool task — 21,608 bytes of hub plus the persistent topic string, per hub.The walk now resolves the shell's class by name and calls its handler with
ZEND_OBJECT_TRANSFER_RELEASEbefore the generic cleanup. No autoload: a release runs outside a request, and a class that is not loaded cannot own anything here anyway.The three handlers in this extension refuse the new kind explicitly. Each branches on TRANSFER and treats everything else as LOAD, so reaching that branch with a NULL
default_fnwould crash. Closure's snapshot is already freed by the walk itself, through thepropertiesmarker. The channel reference and the future's shared state move under protocols of their own — untangling those is a change of its own, not a side effect of this one.Measurements
ext/async: 3 failed, all three pre-existing (__DIR__resolves the symlink, so theirincludeof a php-src helper four levels up misses).tests/phptof true-async/server against a build with this change: 1 failed, pre-existing and unrelated (an h3 test whose--EXPECTF--ends in%A).The leak this exists for, measured on a room transferred into a
ThreadPooltask:definitely lost: 21,648 bytes in 2 blocksbefore,0 bytes in 0 blocksafter.