Fix memory leak: stamp $raw in the endpoint layer instead of a Valinor converter#8
Merged
Merged
Conversation
…alinor converter Long-running processes leaked ~70KB per mapped object: with a non-Closure converter registered (the invokable RawStamper), Valinor's converter pipeline caches a fresh ReflectionFunction + closure per converted value node in a static array GC cannot touch (CuyZ/Valinor#800). Remove the converter and stamp Resource::$raw in ResourceEndpoint::mapResource() via a recursive walker (src/Response/RawStamper.php) that preserves the polymorphic behavior exactly: the top-level Resource gets the full decoded body; nested Resources (Order::$customer, Line::$product, Collection items) get their slice, matched by property name and list position. - $raw stamping is now builder-independent — it works even with a bare MapperBuilder - Add a memory regression test (50 pages must grow memory < 1MB; re-adding an invokable no-op converter makes it fail with ~33MB growth) - Add direct unit tests for the walker (100% MSI) - Document the do-not-register-converters constraint in Client::configureMapperBuilder(), README and CLAUDE.md Fixes #7
f5d3d7b to
fec165d
Compare
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.
Fixes #7
Problem
Long-running processes (e.g. Symfony Messenger workers) leaked ~70KB per mapped object, eventually hitting the PHP
memory_limit. Root cause is upstream CuyZ/Valinor#800: with a converter registered,ValueConverterNodeBuilder::unstack()callsReflection::function()per converted value node, andClosure::fromCallable()on a non-\Closurecallable (our invokableRawStamper) creates a fresh closure each call — so the memoizedReflectionFunctions accumulate forever in a static cache GC cannot touch.One nuance found during verification: the leak requires a non-Closure callable.
Closure::fromCallable()on an existing\Closurereturns the same instance, so plain closure converters get a stable cache key and don't leak (they still pay per-node reflection overhead).Fix
->registerConverter(new RawStamper())fromClient::configureMapperBuilder()and deletesrc/Mapper/RawStamper.php.$rawinResourceEndpoint::mapResource()— the single funnel behindgetOne()/createOne()/updateOne()/getPage()/paginate()— via a new recursive walker (src/Response/RawStamper.php).Resourcegets the full decoded body; every nestedResourcegets its slice, matched by property name and list position — including through non-Resource DTOs (Order::$customer,Collection<T>items,Line::$product, the recursiveCustomer → CustomerContact → Customergraph). All pre-existing$rawtests pass unchanged, andPayload::fromResponse()on embedded resources keeps working.$rawstamping is now builder-independent, so it works even when a consumer passes a bareMapperBuilder. The do-not-register-converters constraint is documented onconfigureMapperBuilder(), in the README production section, and in CLAUDE.md.@paramdocblocks on closures entirely, so the remaining pure-in-practice registrations (Payloadnull-stripper,Identifier::fromReference()) can't be proven pure. The comment inphpstan.neon.distnow explains the real reason.Tests
tests/Client/MapperMemoryLeakTest.php— regression guard: 50 collection pages must growmemory_get_usage()by < 1MB. Memory-based rather than reflecting into Valinor's private static cache, so it survives Valinor refactors and catches any reintroduced per-mapping growth. Verified to discriminate: temporarily re-adding an invokable no-op converter fails it with ~33MB growth.tests/Response/RawStamperTest.php— 10 direct unit tests covering every walker branch (nested resources, descent through non-Resource DTOs, recursive graph, missing/non-array/misaligned slices, literalrawkey). 100% MSI on the walker.Verification
composer phpunit(244 tests),composer analyse(PHPStan level max),composer check-style,vendor/bin/rector --dry-run, andvendor/bin/infection(MSI +19pp over threshold) all pass.