A Room transfers into another thread, and its transit shell releases what it owns - #151
Merged
Conversation
TRANSFER puts a hub reference, a persistent copy of the topic and the retry snapshot into the transit shell; LOAD rebuilds the handle in the destination thread. Before this a task closure that captured a room received an uninitialized object. Known and measured: the transit shell leaks. thread_release_transferred_object (php-async) frees the properties table and the allocation, never free_obj, and the transfer API has no release kind — so the hub reference (21,608 bytes) and the persistent topic (40) are never dropped. HttpServer works around the same gap with http_server_release_worker_shell, which a room has no owner to call.
…nsfer kind A transit shell's C state is now reachable at release (php-src ZEND_OBJECT_TRANSFER_RELEASE, php-async dispatch), so a room drops its hub reference and frees the persistent topic instead of leaking them. Measured on 069: definitely lost falls from 21,648 bytes in 2 blocks to zero. HttpServerConfig releases its frozen snapshot the same way. HttpServer does not: it owns its shells and frees them in http_server_release_worker_shell, so releasing here would double-free. Room::subscriberCount() gets the caveat it always deserved — from a thread that never attached to the hub, which is exactly where a transferred room runs, it returns 0 and that reads the same as a room nobody joined. default_fn now gets 0 for the allocation size in both directions: the default derives it from the handler offset and the property count, and a literal sizeof() would stop covering the object once the stub declares a property.
Contributor
CoverageTotal lines: 81.77% → 81.78% (+0.00 pp)
|
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 and true-async/php-async#232, both merged; php-src is in
true-asyncandtrue-async-stable.A task closure that captured a
Roomused to receive an uninitialized object, because the class registered notransfer_objhandler. It has one now: TRANSFER puts a hub reference, a persistent copy of the topic and the retry snapshot into the transit shell, LOAD rebuilds the handle in the destination thread. This is what a run executing in a pool thread needs before it can be told to stop or handed a person's answer.The shell used to leak.
thread_release_transferred_objectfrees the properties table, the class name and the allocation, and neverfree_obj— so a room's hub reference and its persistent topic had nobody to drop them. The two upstream PRs addZEND_OBJECT_TRANSFER_RELEASE, and this one uses it:Roomdrops the hub reference and frees the topic;HttpServerConfigreleases its frozen snapshot, which leaked the same way;HttpServerrefuses the kind on purpose — it owns its shells and frees them inhttp_server_release_worker_shell, so releasing here would double-free.Room::subscriberCount()gets a caveat in the stub: from a thread that never attached to the hub — exactly where a transferred room runs — it returns 0, which reads the same as a room nobody joined.trySend()andsend()report that thread honestly; the count does not.default_fnis now given 0 for the allocation size in both directions. The default derives it from the handler offset and the property count; a literalsizeof(room_object)is correct only while the stub declares no properties, and stops covering the object the moment one appears.Measurements
069-room-transfers-to-pool.phpt: a pool task publishes through a captured room and a WebSocket client owned by the server thread receives it. Without the handler it fails withRoom is uninitialized— the guard added in A Room holds the topic hub, not the server that minted it #150 turns what would be a NULL dereference into an exception.definitely lost: 21,648 bytes in 2 blocksbefore the release kind,0 bytes in 0 blocksafter.tests/phpt: 1 failed, pre-existing and unrelated —server/h3/045-h3-reload-reactor-pool.phpt, whose--EXPECTF--ends in a%Athat requires a newlinerun-testshas already trimmed.ext/asyncon the same build: 3 failed, all three pre-existing (the ext/async symlink breaks theirincludeof php-src helpers).Re-running any of this needs
-d extension_dir=$PWD/modules, or run-tests loads the installed extension instead of the one just built.