# [PATCH] Raise warning first on "Maximum execution time exceeded"

- Thread: https://externals.io/message/47444
- Original archive: https://news-web.php.net/php.internals/47444
- Started by troels knak-nielsen on 2010-03-22T14:51:40+00:00
- Messages: 20
- 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

---

## #47444 — troels knak-nielsen — 2010-03-22T14:51:40+00:00

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

Hi list.

We log all errors that happens in our production environment, but as
fatal errors can't be handled from within php, we end up with little
information to go on for further debugging. I'm not very familiar with
the php internals code, but I managed to throw in a hack that appears
to work. In the handler function for timeouts (zend_timeout), I raise
a WARNING, sleep for 1 second and then resume normal behavior, which
results in a fatal error. This gives userland code 1 second to log the
error somewhere, which should be sufficient for debugging.

Would like to get feedback as to if this has any hidden problems, but
otherwise I propose that it's included in the project.

-- 
troels

---

## #47445 — Olivier B. — 2010-03-22T15:00:33+00:00

URL: https://externals.io/message/47444#47445 · In reply to: #47444

Hi,

well, maybe we should provide a better behaviour for all fatal errors, no ?
A frequent one I see is the fatal error for calls like "$obj->method()", 
when $obj is not an object.

Isn't possible to allow catching of fatal errors, like other errors ?

Olivier


> Hi list.
>
> We log all errors that happens in our production environment, but as
> fatal errors can't be handled from within php, we end up with little
> information to go on for further debugging. I'm not very familiar with
> the php internals code, but I managed to throw in a hack that appears
> to work. In the handler function for timeouts (zend_timeout), I raise
> a WARNING, sleep for 1 second and then resume normal behavior, which
> results in a fatal error. This gives userland code 1 second to log the
> error somewhere, which should be sufficient for debugging.
>
> Would like to get feedback as to if this has any hidden problems, but
> otherwise I propose that it's included in the project.

---

## #47447 — johannes@php.net — 2010-03-22T15:09:17+00:00

URL: https://externals.io/message/47444#47447 · In reply to: #47445

Hi,


> well, maybe we should provide a better behaviour for all fatal errors, no ?
> A frequent one I see is the fatal error for calls like "$obj->method()", 
> when $obj is not an object.
> 
> Isn't possible to allow catching of fatal errors, like other errors ?

Fatal errors might let the engine in some undefined state. The only safe
thing we can do is terminate the current script. fror other cases we
have catchable fatal errors. Maybe there are a few more cases where
fatal errors can be made catchable fatal errors, but these are case by
case decisions.

johannes

---

## #47446 — johannes@php.net — 2010-03-22T15:14:12+00:00

URL: https://externals.io/message/47444#47446 · In reply to: #47444

> We log all errors that happens in our production environment, but as
> fatal errors can't be handled from within php, we end up with little
> information to go on for further debugging. I'm not very familiar with
> the php internals code, but I managed to throw in a hack that appears
> to work. In the handler function for timeouts (zend_timeout), I raise
> a WARNING, sleep for 1 second and then resume normal behavior, which
> results in a fatal error. This gives userland code 1 second to log the
> error somewhere, which should be sufficient for debugging.

A one second delay is no option there. And what actually happens is that
the warning triggers your custom error handler. After that it sleeps
then it dies.

This also creates a "nice" way to extend the script runtime after the
timeout occurred. aka. making the timeout useless for many scenarios it
was meant for.

johannes

---

## #47448 — troels knak-nielsen — 2010-03-22T15:22:56+00:00

URL: https://externals.io/message/47444#47448 · In reply to: #47446

2010/3/22 Johannes Schlüter <johannes@php.net>:
> A one second delay is no option there. And what actually happens is that
> the warning triggers your custom error handler. After that it sleeps
> then it dies.

What do you mean by "no option"? Otherwise yes, that's what it does.

> This also creates a "nice" way to extend the script runtime after the
> timeout occurred. aka. making the timeout useless for many scenarios it
> was meant for.

I'm not sure I understand. Are you implying that it is somehow
possible to circumvent the second (fatal) error with this change? How?

-- 
troels

---

## #47449 — Herman Radtke — 2010-03-22T15:41:24+00:00

URL: https://externals.io/message/47444#47449 · In reply to: #47448

> What do you mean by "no option"? Otherwise yes, that's what it does.
Using sleep there is not a good practice.  Since the custom error
handler is triggered, there is no need for the sleep call anyways.

> I'm not sure I understand. Are you implying that it is somehow
> possible to circumvent the second (fatal) error with this change? How?

Consider for a second what you are asking the language to do.  The
script has run out of memory and instead of halting you want it to
switch into an error handler to log the error.  What happens when that
error handler creates new variables or an object instance?  Where does
that memory come from?

A better solution: generate a warning for scripts that use some target
% of memory.  I hate to add more ini settings, but for simplicity
there could be a "memory_limit_warn" setting which you can set to
something like "95%".  When a script reaches that memory threshold it
can trigger a warning so you can log it.

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

---

## #47450 — Jille Timmermans — 2010-03-22T15:43:33+00:00

URL: https://externals.io/message/47444#47450 · In reply to: #47446

Op 22-3-2010 16:14, Johannes Schlüter schreef:
> 
>    
> > […]
> A one second delay is no option there. And what actually happens is that
> the warning triggers your custom error handler. After that it sleeps
> then it dies.
>
> This also creates a "nice" way to extend the script runtime after the
> timeout occurred. aka. making the timeout useless for many scenarios it
> was meant for.
>    
This could be made configurable. Shared hosting providers probably want 
to disable the 'overriding' of the timeout. At my work; all code is 
written by our own developers; so there's no evil in there ;) (And it 
would be usefull to give extra debugging information)

-- Jille
> johannes

---

## #47451 — Ferenc Kovacs — 2010-03-22T15:45:06+00:00

URL: https://externals.io/message/47444#47451 · In reply to: #47447

2010/3/22 Johannes Schlüter <johannes@php.net>

> Hi,
>
> 
> > […]
> ?
> > […]
>
> Fatal errors might let the engine in some undefined state. The only safe
> thing we can do is terminate the current script. fror other cases we
> have catchable fatal errors. Maybe there are a few more cases where
> fatal errors can be made catchable fatal errors, but these are case by
> case decisions.
>
> Then why is this snippet working:

<?php
error_reporting(E_ALL);
ini_set('display_errors', 1);

register_shutdown_function(
function (){
echo 111;
}
);

if(ob_get_level()){
 `ob_end_flush()`;
}
echo 123;
 `flush()`;
$obj->func();
echo 456;

script execution can continue on fatal error.
you can use the register_shutdown_function, or the ob_start function
callback param to continue the execution after an uncatchable fatal error.
I think either this kind of errors should be catchable(except the errors in
the engine itself, not in the userland) or the above mentioned two method
shouldn't be working.

Tyrael

> johannes

---

## #47452 — troels knak-nielsen — 2010-03-22T15:55:26+00:00

URL: https://externals.io/message/47444#47452 · In reply to: #47449

> > […]
> Using sleep there is not a good practice.  Since the custom error
> handler is triggered, there is no need for the sleep call anyways.

So control isn't returned to the C-code before the php error handler
finishes. Is that it?


> Consider for a second what you are asking the language to do.  The
> script has run out of memory and instead of halting you want it to
> switch into an error handler to log the error.  What happens when that
> error handler creates new variables or an object instance?  Where does
> that memory come from?

This patch handles timeout, not memory exhaustion.

Although a similar solution for memory exhaustion could be useful as well.

-- 
troels

---

## #47453 — Ferenc Kovacs — 2010-03-22T15:55:57+00:00

URL: https://externals.io/message/47444#47453 · In reply to: #47449

On Mon, Mar 22, 2010 at 4:41 PM, Herman Radtke <hermanradtke@gmail.com>wrote:

> > […]
> Using sleep there is not a good practice.  Since the custom error
> handler is triggered, there is no need for the sleep call anyways.
>
> > […]
>
> Consider for a second what you are asking the language to do.  The
> script has run out of memory and instead of halting you want it to
> switch into an error handler to log the error.  What happens when that
> error handler creates new variables or an object instance?  Where does
> that memory come from?
>
> The out of memory is a good example, but not the only one.
If you reached the memory limit, but the system has available memory, then
it should be handled with a soft limit/hard limit.

> A better solution: generate a warning for scripts that use some target
> % of memory.  I hate to add more ini settings, but for simplicity
> there could be a "memory_limit_warn" setting which you can set to
> something like "95%".  When a script reaches that memory threshold it
> can trigger a warning so you can log it.
>
you can write that in userland with memory_get_usage.
but its just one case from the many.

Tyrael

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

---

## #47454 — Ferenc Kovacs — 2010-03-22T16:01:49+00:00

URL: https://externals.io/message/47444#47454 · In reply to: #47450

2010/3/22 Jille Timmermans <jille@quis.cx>

> Op 22-3-2010 16:14, Johannes Schlüter schreef:
>
>  
> > […]
> This could be made configurable. Shared hosting providers probably want to
> disable the 'overriding' of the timeout. At my work; all code is written by
> our own developers; so there's no evil in there ;) (And it would be usefull
> to give extra debugging information)
>
> If you mentioned it, it would be a good thing, to have a possibility to
control the execution time INCLUDING the time spent waiting for external
resources.
if you run
time php -r 'set_time_limit(5);`sleep 10`;echo "done";';
you will see what I'm talking about.

Tyrael

> -- Jille
>
>  johannes

---

## #47455 — Herman Radtke — 2010-03-22T16:04:15+00:00

URL: https://externals.io/message/47444#47455 · In reply to: #47452

> This patch handles timeout, not memory exhaustion.

Apologies.  My train of thought inexplicably switched over to memory.
I meant to assert that your custom error handler could extend the
length of the script to n extra seconds.  This violates that idea of
the timeout.
-- 
Herman Radtke
hermanradtke@gmail.com | http://hermanradtke.com

---

## #47456 — Ferenc Kovacs — 2010-03-22T16:09:00+00:00

URL: https://externals.io/message/47444#47456 · In reply to: #47455

On Mon, Mar 22, 2010 at 5:04 PM, Herman Radtke <hermanradtke@gmail.com>wrote:

> > […]
>
> Apologies.  My train of thought inexplicably switched over to memory.
> I meant to assert that your custom error handler could extend the
> length of the script to n extra seconds.  This violates that idea of
> the timeout.
>
try:

time php -r 'set_time_limit(1);echo
"start";register_shutdown_function(function(){sleep(10);echo "done";});'

violates what?

Tyrael

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

---

## #47464 — troels knak-nielsen — 2010-03-22T17:55:00+00:00

URL: https://externals.io/message/47444#47464 · In reply to: #47452

> 
> > […]
>
> So control isn't returned to the C-code before the php error handler
> finishes. Is that it?

Just checked, and you're right about this. I've made a new patch,
which addresses this. Again, I have no idea about whether I'm
violating code style (Should zend_timeout_softswitch be declared or
named differently?), so comments on that please?

New patch attached - disregard the previous one.

-- 
troels

---

## #47465 — Scott MacVicar — 2010-03-22T18:10:00+00:00

URL: https://externals.io/message/47444#47465 · In reply to: #47464

> 
> > […]
> 
> Just checked, and you're right about this. I've made a new patch,
> which addresses this. Again, I have no idea about whether I'm
> violating code style (Should zend_timeout_softswitch be declared or
> named differently?), so comments on that please?
> 
> New patch attached - disregard the previous one.
> 

Statics are bad, if its in multithreaded mode and two timeouts happen at once you'll get some funny behaviour. You need to store this in the thread local storage so the flag is per thread.

Scott

---

## #47466 — troels knak-nielsen — 2010-03-22T18:37:09+00:00

URL: https://externals.io/message/47444#47466 · In reply to: #47465

>
> Statics are bad, if its in multithreaded mode and two timeouts happen at once you'll get some funny behaviour. You need to store this in the thread local storage so the flag is per thread.
>

Thanks. My C is severely rusty; Would it simply be a matter of
dropping the "static" modifier, or do I need to get hold of some kind
of handle and attach the flag to that? In which case, what would be
appropriate?

-- 
troels

---

## #47467 — Scott MacVicar — 2010-03-22T18:38:53+00:00

URL: https://externals.io/message/47444#47467 · In reply to: #47466

> 
> > […]
> 
> Thanks. My C is severely rusty; Would it simply be a matter of
> dropping the "static" modifier, or do I need to get hold of some kind
> of handle and attach the flag to that? In which case, what would be
> appropriate?

You'll see places that have EG(...) that's Executor globals and they're per thread or static if its single threaded.

So if you define your new variable wherever timeout_seconds is defined you can add it there. I don't have a checkout to hand so can't point you in the exact place.

Scott

---

## #47468 — troels knak-nielsen — 2010-03-22T19:27:40+00:00

URL: https://externals.io/message/47444#47468 · In reply to: #47467

With the help of Scotts last suggestion, I made some changes. It seems
to work, but then I don't really know how to test it properly. I can
verify that A) it allows userland code the grace period of 1 second
for shutting down and B) shuts down regardless of the userland error
handler returning or not. I have tested by running the following:

    php -r 'function err() { echo "Shutting down"; while (true) {}};
set_error_handler("err"); set_time_limit(2); while (true) {}'

New patch is attached.

-- 
troels

---

## #47474 — Stan Vassilev — 2010-03-23T07:47:46+00:00

URL: https://externals.io/message/47444#47474 · In reply to: #47468

> With the help of Scotts last suggestion, I made some changes. It seems
> to work, but then I don't really know how to test it properly. I can
> verify that A) it allows userland code the grace period of 1 second
> for shutting down and B) shuts down regardless of the userland error
> handler returning or not. I have tested by running the following:




Hi,

Thanks for your patch, but you're going to affect a whole group of users who 
do advanced logging and recovery in the shutdown phase. This is why these 
things have to be considered when they're first added.

A grace period of 1 second seems sufficient on a pristine condition unused 
server, but when your server is loaded, a spike in load may cause a number 
of shutdown handlers to take more than 1 second, and stop middway running, 
causing a lot of unpredictability and trouble for those who rely on this 
feature.

Regards,
Stan Vassilev

---

## #47475 — troels knak-nielsen — 2010-03-23T09:50:15+00:00

URL: https://externals.io/message/47444#47475 · In reply to: #47474

Hi Stan.


> Thanks for your patch, but you're going to affect a whole group of users who
> do advanced logging and recovery in the shutdown phase. This is why these
> things have to be considered when they're first added.

How would they be affected? Current behavior would still be there.

> A grace period of 1 second seems sufficient on a pristine condition unused
> server, but when your server is loaded, a spike in load may cause a number
> of shutdown handlers to take more than 1 second, and stop middway running,
> causing a lot of unpredictability and trouble for those who rely on this
> feature.

So, you mean that a patch that fixes 90% of the cases, but leaves the
10%, is worse than no patch, because of the confusion it might cause?

I guess that could be a valid point; Is it a generally agreed-on
strategy for php?

-- 
troels