[patch] Session RSHUTDOWN, shutdown_destructors() issues

This is a followup to http://bugs.php.net/bug.php?id=54157 . Johannes said I should post here. In the course of my MediaWiki development work, I hit a strange issue involving session_set_save_handler(). PHP's built-in session handler is pretty much useless when you have more than one server serving a given site, so session_set_save_handler() is essential. MediaWiki has a feature allowing it to save sessions via its memcached client. A suitable client object is put in $wgMemc during setup, which is used when the session is closed. A minor change to the way this variable was initialised caused it to disappear from the global symbol table before the session save handler was called. This turned out to be due to a broken algorithm in shutdown_destructors() in zend_execute_API.c. The algorithm basically does this: 1. Delete all objects from the global symbol table which have a zval reference count of 1 2. If any objects were deleted, go to step 1. Obviously the intention is to first delete the references, and then to delete the objects which were referred to. It doesn't work that way: if you have a reference, both symbol table entries point to the same zval, so they both have a reference count of 2, so the algorithm skips both and leaves them intact for RSHUTDOWN. My $wgMemc initialisation change caused the reference count to drop from 2 to 1, so the variable was deleted. Fixing this function to correctly delete all objects from the global symbol table would be easy enough (against 5.3): http://tstarling.com/stuff/shutdown_destructors-sanity.patch But please don't apply that, because it would break all released versions of MediaWiki. We now come to the next strange thing about this code: why is it attempting to delete all objects from the global symbol table anyway? There are plenty of other ways to store objects: function static variables, class static variables, etc. Deleting them from $GLOBALS doesn't stop them from being accessed. And besides, we know that there's no problem with accessing an object after __destruct() has been called on it, because that was the whole point of splitting out shutdown_destructors() in the first place, as a comment in shutdown_executor() explains: /* Removed because this can not be safely done, e.g. in this situation: Object 1 creates object 2 Object 3 holds reference to object 2. Now when 1 and 2 are destroyed, 3 can still access 2 in its destructor, with very problematic results */ /* zend_objects_store_call_destructors(&EG(objects_store) TSRMLS_CC); */ Just removing the whole loop would suit me, as a 5.3.x stopgap measure: http://tstarling.com/stuff/no-global-deletion.patch But that still leaves the problem that the session save handler is called in the strange and scary world of half-shut-down PHP. We don't know what RSHUTDOWN functions have been called before the session RSHUTDOWN, so we don't know what userspace code will work and what won't. A workaround is easy enough: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/83147 but I thought I would be a good open source citizen and come up with a proper solution, against trunk. It is attached and at: http://tstarling.com/stuff/session-pre-deactivate.patch The idea is to introduce a pre-RSHUTDOWN hook, allowing modules to call user code in a relatively sane environment. It's implemented in a way that is closely analogous to the post-deactivate hook. I had trouble testing it because trunk was so broken that it wouldn't run MediaWiki, but the patch appears to work for simple CLI test cases. -- Tim Starling

Attachment: [text/x-diff] session-pre-deactivate.patch