Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » ZTS leak »

ZTS leak

php.internals

Sara Golemon

21 years ago
In looking into a series of leaks regarding tsrm interpreter contexts with George and Wez, we came across a problem with how EG(zend_constants) is created and destroyed. Specifically it's only destroyed once, from zend_shutdown(). However it's created by executor_globals_ctor(). In non-ZTS this isn't a problem since egctor is only called the one time during startup, however when ZTS is enabled egctor gets called repeatedly (including once for every request startup) and the allocated EG(zend_constants) is leaked over and over again. Of lesser criticality (but no less important) rsrc_plist is also leaked by this unbalanced positioning. I temporarily plugged the constants leak (90k+ per request) by using the patch below (though it's inefficient for obvious reasons), but rsrc_plist will take a slightly different approach (heading out the door now so no time to think it through). There's also a 200 byte leak in the thread key, but that's once per engine and not a per-request issue so I'm even less concerned about that one. -Sara Index: Zend/zend.c =================================================================== RCS file: /repository/ZendEngine2/zend.c,v retrieving revision 1.308 diff -u -r1.308 zend.c --- Zend/zend.c 3 Aug 2005 13:30:45 -0000 1.308 +++ Zend/zend.c 29 Aug 2005 21:40:13 -0000 @@ -486,6 +486,7 @@ static void executor_globals_dtor(zend_executor_globals *executor_globals TSRMLS_DC) { zend_ini_shutdown(TSRMLS_C); + zend_shutdown_constants(TSRMLS_C); } @@ -706,7 +707,6 @@ zend_shutdown_extensions(TSRMLS_C); free(zend_version_info); - zend_shutdown_constants(TSRMLS_C); free(GLOBAL_FUNCTION_TABLE); free(GLOBAL_CLASS_TABLE); #ifdef ZTS

Zeev Suraski

21 years ago
At 01:13 30/08/2005, Sara Golemon wrote:
>In looking into a series of leaks regarding tsrm interpreter contexts with >George and Wez, we came across a problem with how EG(zend_constants) is >created and destroyed. > >Specifically it's only destroyed once, from zend_shutdown(). However it's >created by executor_globals_ctor(). In non-ZTS this isn't a problem since >egctor is only called the one time during startup, however when ZTS is >enabled egctor gets called repeatedly (including once for every request >startup) and the allocated EG(zend_constants) is leaked over and over again.
As far as I can tell it's a bug. By the way, we're not talking about a leak that happens too often - unless you're spawning and killing threads very often (in which case TSRM is very inefficient regardless of anything). The leak would happen per-terminated thread.
>Of lesser criticality (but no less important) rsrc_plist is also leaked by >this unbalanced positioning.
Ditto.
>I temporarily plugged the constants leak (90k+ per request) by using the >patch below (though it's inefficient for obvious reasons), but rsrc_plist >will take a slightly different approach (heading out the door now so no time >to think it through).
90K+ per request sounds wrong. I guess that you're either measuring right after the server startup, when the threads are not yet initialized - and then every request initiates a new thread; Or you're using a very odd web server that spawns a thread for each incoming request (bad idea!). If it's the former, then that leak is actually not nearly as bad as you thought, if it's the latter... Fixing that leak should be the least of your worries :)
>There's also a 200 byte leak in the thread key, but that's once per engine >and not a per-request issue so I'm even less concerned about that one. > >-Sara > >Index: Zend/zend.c >=================================================================== >RCS file: /repository/ZendEngine2/zend.c,v >retrieving revision 1.308 >diff -u -r1.308 zend.c >--- Zend/zend.c 3 Aug 2005 13:30:45 -0000 1.308 >+++ Zend/zend.c 29 Aug 2005 21:40:13 -0000 >@@ -486,6 +486,7 @@ > static void executor_globals_dtor(zend_executor_globals *executor_globals >TSRMLS_DC) > { > zend_ini_shutdown(TSRMLS_C); >+ zend_shutdown_constants(TSRMLS_C); > } > > >@@ -706,7 +707,6 @@ > zend_shutdown_extensions(TSRMLS_C); > free(zend_version_info); > >- zend_shutdown_constants(TSRMLS_C); > free(GLOBAL_FUNCTION_TABLE); > free(GLOBAL_CLASS_TABLE); > #ifdef ZTS
Are you sure it's not leaking the global constants table with the zend_shutdown_constants() removed from zend_shutdown()? Zeev

Sara Golemon

21 years ago
> As far as I can tell it's a bug. By the way, we're not talking about a > leak that happens too often - unless you're spawning and killing threads > very often (in which case TSRM is very inefficient regardless of > anything). The leak would happen per-terminated thread. >
Good call. The backstory is that George noticed a leak in the PHP::Interpreter Perl module and I saw the same thing in my Runkit_Sandbox class. Both of these spawn a thread context when they instantiate so in this case request == thread (and yes, I acknowledge it's an edge case relative to real-world uses, but it does highlight an otherwise unnoticed problem).
>>I temporarily plugged the constants leak (90k+ per request) by using the >>patch below (though it's inefficient for obvious reasons), but rsrc_plist >>will take a slightly different approach (heading out the door now so no >>time >>to think it through). > 90K+ per request sounds wrong. I guess that you're either measuring right > after the server startup, when the threads are not yet initialized - and > then every request initiates a new thread; Or you're using a very odd web > server that spawns a thread for each incoming request (bad idea!). If > it's the former, then that leak is actually not nearly as bad as you > thought, if it's the latter... Fixing that leak should be the least of > your worries :) >
You're right (see above).
> Are you sure it's not leaking the global constants table with the > zend_shutdown_constants() removed from zend_shutdown()? >
valgrind says no. To be honest I havn't gone looking to see where the global table is cleaned up...Just got back home... -Sara

Zeev Suraski

21 years ago
At 04:03 30/08/2005, Sara Golemon wrote:
>>Are you sure it's not leaking the global constants table with the >>zend_shutdown_constants() removed from zend_shutdown()? >valgrind says no. To be honest I havn't gone looking to see where the >global table is cleaned up...Just got back home...
If valgrind says no then I wouldn't argue :) Sounds a bit odd but if there's one messy piece of code in PHP it's the thread-safe initialization/destruction, so I wouldn't rule anything out. Zeev

Sara Golemon

21 years ago
>>@@ -706,7 +707,6 @@ >> zend_shutdown_extensions(TSRMLS_C); >> free(zend_version_info); >> >>- zend_shutdown_constants(TSRMLS_C); >> free(GLOBAL_FUNCTION_TABLE); >> free(GLOBAL_CLASS_TABLE); >> #ifdef ZTS > > Are you sure it's not leaking the global constants table with the > zend_shutdown_constants() removed from zend_shutdown()? >
Incidently, I just opened my terminal back up and noticed the next three lines immediately following the code block above are: zend_destroy_rsrc_list(&EG(persistent_list) TSRMLS_CC); zend_hash_destroy(GLOBAL_CONSTANTS_TABLE); free(GLOBAL_CONSTANTS_TABLE); Which is of course why there's no leak when zend_shutdown_constants(TSRMLS_C); is moved out of zend_shutdown().