Hi!
I was looking at bug https://bugs.php.net/bug.php?id=67644 and it looks
like we have a bit of a problem with output buffering and dtors on
shutdown. Basically, right now our code looks like this:
/* 2. Call all possible __destruct() functions */
zend_try {
zend_call_destructors(TSRMLS_C);
} zend_end_try();
/* 3. Flush all output buffers */
zend_try {
// And here we go flushing output buffers
Now, since ob functions can have userland handlers, these handlers can
create new objects. And these objects will not benefit from orderly dtor
round that zend_call_destructors(TSRMLS_C) is providing - instead their
dtors will be called from zend_objects_store_free_object_storage() and
by then their environment may already be ruined and their dtors can not
run properly.
OTOH, moving __destruct after output buffers may not be good either, as
dtors may output something.
The problem seems to be resolved if I either duplicate
zend_call_destructors(TSRMLS_C); after the output buffers flushing or put
zend_objects_store_mark_destructed(&EG(objects_store) TSRMLS_CC);
there.
Of course, the second solution has the downside of not calling the dtors
of objects that were created while flushing ob buffers. The
zend_call_destructors would work but since output buffering is supposed
to be shut down by then, I wonder if it won't also have bad consequences
if these dtors do something with output buffering.
I'm leaning towards the second solution - if you create the objects so
late on shutdown stage, you shouldn't really expect them to be destroyed
normally, otherwise the cycle would never end. However, we could
consider the first option too. Any thoughts?
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi!
I was looking at bug https://bugs.php.net/bug.php?id=67644 and it looks
like we have a bit of a problem with output buffering and dtors on
shutdown. Basically, right now our code looks like this:/* 2. Call all possible __destruct() functions */
zend_try {
zend_call_destructors(TSRMLS_C);
} zend_end_try();/* 3. Flush all output buffers */
zend_try {// And here we go flushing output buffers
Now, since ob functions can have userland handlers, these handlers can
create new objects. And these objects will not benefit from orderly dtor
round that zend_call_destructors(TSRMLS_C) is providing - instead their
dtors will be called from zend_objects_store_free_object_storage() and
by then their environment may already be ruined and their dtors can not
run properly.OTOH, moving __destruct after output buffers may not be good either, as
dtors may output something.The problem seems to be resolved if I either duplicate
zend_call_destructors(TSRMLS_C); after the output buffers flushing or put
zend_objects_store_mark_destructed(&EG(objects_store) TSRMLS_CC);
there.Of course, the second solution has the downside of not calling the dtors
of objects that were created while flushing ob buffers. The
zend_call_destructors would work but since output buffering is supposed
to be shut down by then, I wonder if it won't also have bad consequences
if these dtors do something with output buffering.I'm leaning towards the second solution - if you create the objects so
late on shutdown stage, you shouldn't really expect them to be destroyed
normally, otherwise the cycle would never end. However, we could
consider the first option too. Any thoughts?
This is just a thought; could we delay the call to zend_call_destructors ONLY IF there’s output buffering taking place (i.e. ob_get_level() > 0)?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi!
This is just a thought; could we delay the call to
zend_call_destructorsONLY IF there’s output buffering taking place
(i.e.ob_get_level()> 0)?
That wouldn't help - imagine this:
- ob_start is set
- shutdown is starting
- ob functions shut down, call function foo
- function foo creates an object of class FooBar
- ob shutdown ends, all output is flushed, etc.
- FooBar::__destruct is run and tries to output something
That scenario still may have a problem. I'll check more into if it's
really a big deal outputting after OB shutdown (after all, some other
things may lead to it too) but conditioning dtor move does not solve
this problem. If it works this way we may as well move them there
permanently.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi!
This is just a thought; could we delay the call to
zend_call_destructorsONLY IF there’s output buffering taking place
(i.e.ob_get_level()> 0)?That wouldn't help - imagine this:
- ob_start is set
- shutdown is starting
- ob functions shut down, call function foo
- function foo creates an object of class FooBar
- ob shutdown ends, all output is flushed, etc.
- FooBar::__destruct is run and tries to output something
So let it output something ...
Trying to output something in a destructor after flushing the output seems rather fishy; at the same time I’m quite aware that it’s impossible to predict what some developers would expect to happen in such cases.
That scenario still may have a problem. I'll check more into if it's
really a big deal outputting after OB shutdown (after all, some other
things may lead to it too) but conditioning dtor move does not solve
this problem. If it works this way we may as well move them there
permanently.
Yeah, I can see now how my suggestion doesn’t really need the condition then :)
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
On Sun, Aug 31, 2014 at 3:33 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
I was looking at bug https://bugs.php.net/bug.php?id=67644 and it looks
like we have a bit of a problem with output buffering and dtors on
shutdown. Basically, right now our code looks like this:/* 2. Call all possible __destruct() functions */ zend_try { zend_call_destructors(TSRMLS_C); } zend_end_try(); /* 3. Flush all output buffers */ zend_try {// And here we go flushing output buffers
Now, since ob functions can have userland handlers, these handlers can
create new objects. And these objects will not benefit from orderly dtor
round that zend_call_destructors(TSRMLS_C) is providing - instead their
dtors will be called from zend_objects_store_free_object_storage() and
by then their environment may already be ruined and their dtors can not
run properly.OTOH, moving __destruct after output buffers may not be good either, as
dtors may output something.The problem seems to be resolved if I either duplicate
zend_call_destructors(TSRMLS_C); after the output buffers flushing or put
zend_objects_store_mark_destructed(&EG(objects_store) TSRMLS_CC);
there.Of course, the second solution has the downside of not calling the dtors
of objects that were created while flushing ob buffers. The
zend_call_destructors would work but since output buffering is supposed
to be shut down by then, I wonder if it won't also have bad consequences
if these dtors do something with output buffering.I'm leaning towards the second solution - if you create the objects so
late on shutdown stage, you shouldn't really expect them to be destroyed
normally, otherwise the cycle would never end. However, we could
consider the first option too. Any thoughts?
Instead of iterating through all objects and setting a flag, can't we set a
global flag that object dtors are not called after this point? This both
solves the issue of new objects being created after the fact and makes
shutdown less expensive.
Nikita
Hi!
Instead of iterating through all objects and setting a flag, can't we
set a global flag that object dtors are not called after this point?
We could, but that would probably break some code that expects dtors to
be actually called. E.g. in the bug, the library there seems to do a lot
from the dtor, and if we just shut it down it may break the library. I
don't think we can do this in 5.x.
And I don't think we could get rid of the object flag in that case,
since there could be cases of circular links that may still require this
flag to avoid double calling of dtor (even not on shutdown).
This both solves the issue of new objects being created after the fact
and makes shutdown less expensive.
I'm not sure expense of the shutdown is that big a deal though. Bigger
deal is avoiding crashes on the shutdown.
It seems to be that disabling dtors on all objects there is a bit too
harsh - after all, for most of them it is fine, it's just those that
linger after the OB code run and have dependencies on other objects
that create trouble. Another dtor round, for example, should resolve it,
or just marking only those that linger as destroyed. This seems to have
less impact that just banning dtors altogether.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi,
Another possible behaviour would be to trigger a fatal when trying to
access a partially destroyed object in ob_start.
It took me a while to actually figure out why the memory was corrupted
when doing stuff in ob_start() (even before playing with the AMQP lib).
Having a fatal would allow me to save a lot of time, and it should not
introduce any BC issue since any existing code accessing a partially
destroyed object could randomly crash at some point.
Jocelyn
Le 31/08/2014 20:48, Stas Malyshev a écrit :
Hi!
Instead of iterating through all objects and setting a flag, can't we
set a global flag that object dtors are not called after this point?
We could, but that would probably break some code that expects dtors to
be actually called. E.g. in the bug, the library there seems to do a lot
from the dtor, and if we just shut it down it may break the library. I
don't think we can do this in 5.x.And I don't think we could get rid of the object flag in that case,
since there could be cases of circular links that may still require this
flag to avoid double calling of dtor (even not on shutdown).This both solves the issue of new objects being created after the fact
and makes shutdown less expensive.
I'm not sure expense of the shutdown is that big a deal though. Bigger
deal is avoiding crashes on the shutdown.
It seems to be that disabling dtors on all objects there is a bit too
harsh - after all, for most of them it is fine, it's just those that
linger after the OB code run and have dependencies on other objects
that create trouble. Another dtor round, for example, should resolve it,
or just marking only those that linger as destroyed. This seems to have
less impact that just banning dtors altogether.
Hi!
I've created a proposed fix for #67644:
https://github.com/php/php-src/pull/798
It is the most conservative one which I'm thinking of putting into 5.4,
so that at least we won't have segfaults. If we want to improve upon it,
we can do that in 5.5/5.6, but I'd like at least plug the segfaults in
5.4 for now.
Any objections to this or better proposals?
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi,
One line fix, nice :)
A quick question about how resources are freed :
if some variables are computed inside a singleton and read through a
myclass::instance()->get_my_variable(), should we expect to have those
variables available when calling ob_start() callbacks ?
From experiment, they are still available, but since
zend_call_destructors is called before ob_start(), I wonder if this is a
safe behaviour.
Thanks,
Jocelyn
Le 02/09/2014 09:56, Stas Malyshev a écrit :
Hi!
I've created a proposed fix for #67644:
https://github.com/php/php-src/pull/798
It is the most conservative one which I'm thinking of putting into 5.4,
so that at least we won't have segfaults. If we want to improve upon it,
we can do that in 5.5/5.6, but I'd like at least plug the segfaults in
5.4 for now.Any objections to this or better proposals?
Hi!
One line fix, nice :)
A quick question about how resources are freed :
if some variables are computed inside a singleton and read through a
myclass::instance()->get_my_variable(), should we expect to have those
variables available when callingob_start()callbacks ?From experiment, they are still available, but since
zend_call_destructors is called beforeob_start(), I wonder if this is a
safe behaviour.
Generally, it would depend on when they were created. If they were
created before shutdown stage and you try to access them in OB shutdown
stage, it may be too late and the dtor may have been called for them
already. Doing too much stuff on shutdown is not really a good idea, as
you can not rely on resources being there. If you really need to do
something on shutdown, I'd advise register_shutdown_function() as it is
called first, before anything is really shut down. Relying on OB
handlers shutdown is not a good idea as by then at least part of the
shutdown happened. If you do something with OB handlers, I'd propose to
add a shutdown function that flushes the OB, so that you are sure
everything is wrapped up while the engine is still in predictable state
and not half-shutdown.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi Stas,
Actually I would love flushing ob_start() in my own shutdown function,
but since my application is a library called in auto_prepend_file, I
have to protect ob_start() from being flushed too early by some wrong
code in the client application.
Hence I'm removing the PHP_OUTPUT_HANDLER_CLEANABLE,
PHP_OUTPUT_HANDLER_FLUSHABLE & PHP_OUTPUT_HANDLER_REMOVABLE, but then
I'm not able myself to flush the buffer :(
A great "new" feature would be to optionally give a name to a buffer,
and then allow to flush this buffer only if its name is passed through
ob_end_clean / ob_end_flush / ...
Thanks,
Jocelyn
Le 02/09/2014 21:29, Stas Malyshev a écrit :
Hi!
One line fix, nice :)
A quick question about how resources are freed :
if some variables are computed inside a singleton and read through a
myclass::instance()->get_my_variable(), should we expect to have those
variables available when callingob_start()callbacks ?From experiment, they are still available, but since
zend_call_destructors is called beforeob_start(), I wonder if this is a
safe behaviour.Generally, it would depend on when they were created. If they were
created before shutdown stage and you try to access them in OB shutdown
stage, it may be too late and the dtor may have been called for them
already. Doing too much stuff on shutdown is not really a good idea, as
you can not rely on resources being there. If you really need to do
something on shutdown, I'd adviseregister_shutdown_function()as it is
called first, before anything is really shut down. Relying on OB
handlers shutdown is not a good idea as by then at least part of the
shutdown happened. If you do something with OB handlers, I'd propose to
add a shutdown function that flushes the OB, so that you are sure
everything is wrapped up while the engine is still in predictable state
and not half-shutdown.
Hi!
On Tue, Sep 2, 2014 at 12:56 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
I've created a proposed fix for #67644:
Looks like this fix breaks some tests, so I'm not feeling good putting it
into 5.4. I'll revert it for now and try to figure out a better solution
later.