This issue is automatically created based on existing pull request: #41310: Shutdown functions of SessionManager and Webapi ErrorProcessor no longer keep them alive
Description (*)
SessionManager::registerShutdown() and Webapi\ErrorProcessor::registerShutdownFunction() pass [$this, ...] to register_shutdown_function(). PHP keeps every registered shutdown callback until the process exits and has no way to remove one. So every session and web API error processor, and everything they reference, stays in memory until the process ends, even after nothing else uses them.
In a normal request that costs nothing, because the objects live until shutdown anyway. In a long-running process that creates them repeatedly, it's a leak. The integration test framework is the clearest case: @magentoAppIsolation (on by default for controller tests) reinitializes the application after each test, and each new application starts a session and creates an ErrorProcessor. Both are Interceptors, so each one keeps the ObjectManager of the application it came from alive, and with it that application's whole object graph. gc_collect_cycles() can't free any of it, because the shutdown list holds a reference from outside the cycle.
Measured on a third-party module's integration suite (528 tests) on 2.4.9, run in one PHP process:
|
Peak memory |
Result |
| Before |
9.07 GB (about 15 MB kept per isolated test) |
runs out of memory with memory_limit=8G |
| After |
1.77 GB |
same 528 tests pass, same skips, same wall time |
A stock core class shows the same growth, for example Magento/Catalog/Controller/Product/CompareTest keeps 8–16 MB per test.
This PR makes both callbacks hold a WeakReference and do nothing when the object is gone. When the object is still alive at shutdown, which is always the case in a regular request, the behaviour is unchanged: writeClose() and apiShutdownFunction() run exactly as before.
A side effect: after a fatal error in a long-running process, the web API error JSON was printed once for every error processor ever created (for example, the "Allowed memory size exhausted" message repeated many times at the end of an integration run). Now only processors that are still in use report it.
Related Pull Requests
None.
Fixed Issues (if relevant)
No existing issue found.
Manual testing scenarios (*)
- Run an app-isolated integration test class and watch memory per test, e.g.
php -d memory_limit=-1 vendor/bin/phpunit -c dev/tests/integration/phpunit.xml --log-events-verbose-text /tmp/events.txt dev/tests/integration/testsuite/Magento/Catalog/Controller/Product/CompareTest.php. The memory recorded on each Test Finished line grows 8–16 MB per test before the change and stays flat after it.
- Storefront: log in as a customer, add a product to the cart and reload. The session is still written at the end of the request, and the cart and login persist.
- Web API: make a REST request that triggers a PHP fatal error (for example, a temporary
trigger_error('x', E_USER_ERROR) in a service method). The error response is still rendered by ErrorProcessor::apiShutdownFunction() as before.
Questions or comments
The new session test is its own class (SessionManagerShutdownTest) because SessionManagerTest is skipped as a whole (MAGETWO-34751). Both new tests fail on the current code and pass with the change. The changed files have the same phpcs (Magento2 standard) findings as before, and the new test file has none.
Contribution checklist (*)
This issue is automatically created based on existing pull request: #41310: Shutdown functions of SessionManager and Webapi ErrorProcessor no longer keep them alive
Description (*)
SessionManager::registerShutdown()andWebapi\ErrorProcessor::registerShutdownFunction()pass[$this, ...]toregister_shutdown_function(). PHP keeps every registered shutdown callback until the process exits and has no way to remove one. So every session and web API error processor, and everything they reference, stays in memory until the process ends, even after nothing else uses them.In a normal request that costs nothing, because the objects live until shutdown anyway. In a long-running process that creates them repeatedly, it's a leak. The integration test framework is the clearest case:
@magentoAppIsolation(on by default for controller tests) reinitializes the application after each test, and each new application starts a session and creates anErrorProcessor. Both are Interceptors, so each one keeps the ObjectManager of the application it came from alive, and with it that application's whole object graph.gc_collect_cycles()can't free any of it, because the shutdown list holds a reference from outside the cycle.Measured on a third-party module's integration suite (528 tests) on 2.4.9, run in one PHP process:
memory_limit=8GA stock core class shows the same growth, for example
Magento/Catalog/Controller/Product/CompareTestkeeps 8–16 MB per test.This PR makes both callbacks hold a
WeakReferenceand do nothing when the object is gone. When the object is still alive at shutdown, which is always the case in a regular request, the behaviour is unchanged:writeClose()andapiShutdownFunction()run exactly as before.A side effect: after a fatal error in a long-running process, the web API error JSON was printed once for every error processor ever created (for example, the "Allowed memory size exhausted" message repeated many times at the end of an integration run). Now only processors that are still in use report it.
Related Pull Requests
None.
Fixed Issues (if relevant)
No existing issue found.
Manual testing scenarios (*)
php -d memory_limit=-1 vendor/bin/phpunit -c dev/tests/integration/phpunit.xml --log-events-verbose-text /tmp/events.txt dev/tests/integration/testsuite/Magento/Catalog/Controller/Product/CompareTest.php. The memory recorded on eachTest Finishedline grows 8–16 MB per test before the change and stays flat after it.trigger_error('x', E_USER_ERROR)in a service method). The error response is still rendered byErrorProcessor::apiShutdownFunction()as before.Questions or comments
The new session test is its own class (
SessionManagerShutdownTest) becauseSessionManagerTestis skipped as a whole (MAGETWO-34751). Both new tests fail on the current code and pass with the change. The changed files have the same phpcs (Magento2 standard) findings as before, and the new test file has none.Contribution checklist (*)