# [patch] Session RSHUTDOWN, shutdown_destructors() issues

- Thread: https://externals.io/message/51709
- Original archive: https://news-web.php.net/php.internals/51709
- Started by Tim Starling on 2011-03-16T04:40:01+00:00
- Messages: 7
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #51709 — Tim Starling — 2011-03-16T04:40:01+00:00

URL: https://externals.io/message/51709#51709

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

---

## #51713 — Ferenc Kovacs — 2011-03-16T08:26:55+00:00

URL: https://externals.io/message/51709#51713 · In reply to: #51709

2011/3/16 Tim Starling <tstarling@wikimedia.org>

> 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
>
> --
> 
> 
>

Hi.

Could you please also open a ticket in the bug tracker?
thanks

Tyrael

---

## #51718 — Herman Radtke — 2011-03-17T00:55:31+00:00

URL: https://externals.io/message/51709#51718 · In reply to: #51713

> Could you please also open a ticket in the bug tracker?
> thanks

He did: http://bugs.php.net/bug.php?id=54157

Now he is bringing the discussion here per Johannes.  I too have
experienced Tim's problem and am interested in getting a discussion
going on what can be done to solve it.


-- 
Herman Radtke
hermanradtke@gmail.com | http://hermanradtke.com

---

## #51731 — Tim Starling — 2011-03-20T01:52:50+00:00

URL: https://externals.io/message/51709#51731 · In reply to: #51709

I guess I'm suffering from some tl;dr here. Here's the short version:
please apply the attached patch to the 5.3 branch. It removes some
code which is broken and unnecessary.

---

## #51732 — Stas Malyshev — 2011-03-20T03:30:55+00:00

URL: https://externals.io/message/51709#51732 · In reply to: #51731

Hi!

> I guess I'm suffering from some tl;dr here. Here's the short version:
> please apply the attached patch to the 5.3 branch. It removes some
> code which is broken and unnecessary.

I'm not sure it's a good idea. That code is there for a reason, 
specifically: http://svn.php.net/viewvc?view=revision&revision=216213
Did you check that this test still works when you apply your patch?
-- 
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

---

## #51733 — Tim Starling — 2011-03-21T00:20:44+00:00

URL: https://externals.io/message/51709#51733 · In reply to: #51732

> Hi!
>
> > […]
>
> I'm not sure it's a good idea. That code is there for a reason,
> specifically: http://svn.php.net/viewvc?view=revision&revision=216213
> Did you check that this test still works when you apply your patch?

It doesn't work, because my patch is just a revert.

That bug (36759) doesn't seem to have anything to do with global
variables, so deleting global variables seems like an odd response to it.

r216213 fixes that one test case, but it seems like the actual
complaint there is that objects stored in member variables are
destroyed before their parent objects. r216213 fixes this by assuming
that all parent objects will be in the global symbol table. It deletes
global symbol table entries, so that only objects which are referred
to from somewhere else will survive until the
zend_objects_store_call_destructors().

If you take Dmitry's test case and replace this:

$y = new Bar();
$x = new Foo($y);

with this:

function foo() {
    static $x, $y;
    $y = new Bar();
    $x = new Foo($y);
}
foo();

then the bug is still seen. Or indeed, because of the logic error I've
discussed, this also shows an incorrect destructor order:

$y = new Bar();
$x = new Foo($y);
$z = $x;

Can you reopen bug 36759 please?

-- Tim Starling

---

## #51734 — Stas Malyshev — 2011-03-21T00:33:41+00:00

URL: https://externals.io/message/51709#51734 · In reply to: #51733

Hi!

> Can you reopen bug 36759 please?

Please add these comments and other relevant info to the bug, I'll 
reopen it.

-- 
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227