Everything

php

Home » Project » Newsgroups » php.internals » [RFC] [Discussion] Error backtraces v2 »

[RFC] [Discussion] Error backtraces v2

php.internals

Eric Norris

1 year ago

Hello yet again internals,

I'd like to formally propose a continuation of the original error
backtraces RFC (https://wiki.php.net/rfc/error_backtraces):

https://wiki.php.net/rfc/error_backtraces_v2

We recently had a production incident that might have been shortened,
had we been able to identify an errant infinite loop in our code from
the backtrace of a PHP timeout error. It's likely that there are
plenty of other error conditions that would be easier to debug with a
stacktrace, too.

The original RFC is inactive, but I've read the proposal and resulting
discussion and I believe that @nikic's suggestion is a viable way to
address the RFC's issues. I've implemented that suggestion here:

https://github.com/php/php-src/pull/17056

The RFC currently has two voting choices, the first is to accept an
INI setting to configure an error mask for backtraces, and the second
is to choose the default value for the error mask.

Please take a look at let me know what you think.

Thank you!

Tim Düsterhus

1 year ago

Hi

Thank you for your RFC. This time I'm making sure to comment already
during the discussion period :-)

On 12/5/24 21:57, Eric Norris wrote:

> https://wiki.php.net/rfc/error_backtraces_v2
>
> The RFC currently has two voting choices, the first is to accept an
> INI setting to configure an error mask for backtraces, and the second
> is to choose the default value for the error mask.

You are not restricted to Yes/No votes for the voting widgets. I suggest
to rephrase the second voting widget to:

"What should the default value of the 'error_backtrace_recording' INI be?"

- E_FATAL_ERRORS
- 0

This avoids the complicated sentence above the voting widget.

The “php.ini Defaults” section should probably refer to the voting
widget as well.

The first voting widget would probably be better phrased as:

"Add the 'error_backtrace_recording' INI as described?"

---

As for the RFC contents, I had a look at the previous discussion. Dan
remarked that adding the backtrace to `error_get_last()` would keep any
parameters to functions in the stack trace alive until the next error
occurs. This can easily break RAII patterns, such as this example:
https://3v4l.org/i6EOv

<?php
class Lock
{
public function __construct() { echo "Locked\n"; }
public function __destruct() { echo "Unlocked\n"; }
}

function do_while_locked(Lock $lock)
{
// The exception implicitly keeps the lock alive.
$GLOBALS['x'] = new Exception();
echo "Doing something\n";
}

function foo()
{
$lock = new Lock();
do_while_locked($lock);
echo "Done\n";
}

foo();
echo "After\n";

Normally one would expect the "Unlocked" to be printed between "Done"
and "After", but this is not the case, because the Exception in the
global keeps a reference to $lock within its backtrace. This would be
the same if the Exception would instead be a `trigger_error()` that
remains available via `error_get_last()`.

In other words, we would have a INI setting that changes application
behavior in subtle ways, which is a no-no.

For the previous RFC this apparently was a non-issue, because the stack
was stored as a string. But for your RFC this is an issue, because based
on the tests it's stored as an array.

At the very least this gotcha should clearly be noted in the RFC text.

Best regards
Tim Düsterhus

Eric Norris

1 year ago

Hello,

On Thu, Dec 5, 2024 at 5:31 PM Tim Düsterhus <tim@bastelstu.be> wrote:

>
> Hi
>
> Thank you for your RFC. This time I'm making sure to comment already
> during the discussion period :-)

Haha, thank you! I appreciate the feedback as always.

>
> On 12/5/24 21:57, Eric Norris wrote:
> > https://wiki.php.net/rfc/error_backtraces_v2
> >
> > The RFC currently has two voting choices, the first is to accept an
> > INI setting to configure an error mask for backtraces, and the second
> > is to choose the default value for the error mask.
>
> You are not restricted to Yes/No votes for the voting widgets. I suggest
> to rephrase the second voting widget to:
>
> "What should the default value of the 'error_backtrace_recording' INI be?"
>
> - E_FATAL_ERRORS
> - 0
>
> This avoids the complicated sentence above the voting widget.

Good point.

>
> The “php.ini Defaults” section should probably refer to the voting
> widget as well.
>
> The first voting widget would probably be better phrased as:
>
> "Add the 'error_backtrace_recording' INI as described?"

I'm a little nervous about this change, because I was hoping to be a
little more open-ended. For example, I'm not tied to the name
'error_backtrace_recording', so I wouldn't want someone to vote 'no'
because they didn't like the setting name. Secondly, maybe we don't
want the zend global (which I'll actually get to in a bit), and
similarly I wouldn't want someone to vote 'no' because of that; I
don't actually care about whether it's a global or not, I only care
about getting backtraces for fatal errors.

Maybe this is a little bit of an overreaction from my previous RFC, in
that I'm trying to give myself more room for differing implementations
while achieving my primary goal. What are your thoughts here?

> <snip>
> Normally one would expect the "Unlocked" to be printed between "Done"
> and "After", but this is not the case, because the Exception in the
> global keeps a reference to $lock within its backtrace. This would be
> the same if the Exception would instead be a `trigger_error()` that
> remains available via `error_get_last()`.
>
> In other words, we would have a INI setting that changes application
> behavior in subtle ways, which is a no-no.

Great example, thank you. Since you are stating this is problematic -
and I don't necessarily disagree - does this make allowing backtraces
as implemented for non-fatal errors a non-starter? Presumably having a
backtrace for a fatal error doesn't matter since everything will soon
be destroyed in the process of shutting down.

> For the previous RFC this apparently was a non-issue, because the stack
> was stored as a string. But for your RFC this is an issue, because based
> on the tests it's stored as an array.

That's a good point; I hadn't made that connection when I made the
change to use an array (which was an add-on suggestion by nikic,
separate from the error mask suggestion). I think this is
fundamentally an issue even without the change to error_get_last,
since I'm storing the backtrace in a zend global.

I would have preferred to make this a parameter to zend_error_cb, but
I thought that changing the signature of that method might be too much
of a backwards compatibility break (though this would only affect
extensions). Making it a parameter would shorten the lifetime of the
trace, and PHP's zend_error_cb could store that trace as a string for
error_get_last.

> At the very least this gotcha should clearly be noted in the RFC text.

Right, but of course if that means the RFC will fail, I'd rather
implement it differently upfront.

Thinking about the options I see here:

- We could make error backtraces non-configurable and only for fatal
errors, which I believe means we can ignore the lifecycle issue, since
application code will switch into shutdown mode. We should still
mention the caveat, however, as it still means that the destructors
won't fire until after all of the application's shutdown code has run.
- We could keep error backtraces configurable as it is now in the RFC,
but change zend_error_cb to take the trace as an optional parameter,
and not use a global. php_error_cb could store the trace as a string,
which could be used by error_get_last. This feels slightly better to
me than the next option.
- We could keep error backtraces configurable as it is now in the RFC,
but store a string in the zend global. For some reason, this doesn't
feel "right" to me, since we'd be giving up the structured data in the
array.
- We could remove arguments from the backtrace. This feels wrong as
well, since it would make them different from exceptions.
- We could keep the status quo, and note the caveat.

Are there any other options people can think of? What are internals'
preferences for this?

Tim Düsterhus

1 year ago

Hi

Am 2024-12-06 02:08, schrieb Eric Norris:

>> The “php.ini Defaults” section should probably refer to the voting
>> widget as well.
>>
>> The first voting widget would probably be better phrased as:
>>
>> "Add the 'error_backtrace_recording' INI as described?"
>
> I'm a little nervous about this change, because I was hoping to be a
> little more open-ended. For example, I'm not tied to the name
> 'error_backtrace_recording', so I wouldn't want someone to vote 'no'
> because they didn't like the setting name. Secondly, maybe we don't
> want the zend global (which I'll actually get to in a bit), and
> similarly I wouldn't want someone to vote 'no' because of that; I
> don't actually care about whether it's a global or not, I only care
> about getting backtraces for fatal errors.

I believe RFCs should be sufficiently precise so that one is able to
implement the proposal reasonably unambiguously based on the RFC text
alone. Given that you are concerned about the INI setting causing the
RFC not to pass, it should be evident that this is such an important
topic that it needs the RFC to be clear about what is proposed for
voters to be able to vote on the full picture.

>> Normally one would expect the "Unlocked" to be printed between "Done"
>> and "After", but this is not the case, because the Exception in the
>> global keeps a reference to $lock within its backtrace. This would be
>> the same if the Exception would instead be a `trigger_error()` that
>> remains available via `error_get_last()`.
>>
>> In other words, we would have a INI setting that changes application
>> behavior in subtle ways, which is a no-no.
>
> Great example, thank you. Since you are stating this is problematic -
> and I don't necessarily disagree - does this make allowing backtraces
> as implemented for non-fatal errors a non-starter? Presumably having a
> backtrace for a fatal error doesn't matter since everything will soon
> be destroyed in the process of shutting down.

Yes, I agree that having a stacktracke for fatal errors is a non-issue.
For non-fatal errors the INI setting definitely is a problem.
Unconditionally having a stacktrace for non-fatal errors I'm am not yet
sure if this would be a problem or if it would be fine if it is a
RFC-agreed-upon (breaking) change. Given that it's common to turn
warnings and notices into Exceptions and thus to treat them as hard
errors, I'd be inclined to say that the value-add of having backtraces
for them is fairly small compared to the possible impact and that it
makes sense not to include this in the RFC at all.

>> For the previous RFC this apparently was a non-issue, because the
>> stack
>> was stored as a string. But for your RFC this is an issue, because
>> based
>> on the tests it's stored as an array.
>
> I would have preferred to make this a parameter to zend_error_cb, but
> I thought that changing the signature of that method might be too much
> of a backwards compatibility break (though this would only affect
> extensions). Making it a parameter would shorten the lifetime of the

Making breaking changes to the internal API is generally fine if there
is a reason for making the change.

Best regards
Tim Düsterhus

Eric Norris

1 year ago

> Yes, I agree that having a stacktracke for fatal errors is a non-issue.
> For non-fatal errors the INI setting definitely is a problem.
> Unconditionally having a stacktrace for non-fatal errors I'm am not yet
> sure if this would be a problem or if it would be fine if it is a
> RFC-agreed-upon (breaking) change. Given that it's common to turn
> warnings and notices into Exceptions and thus to treat them as hard
> errors, I'd be inclined to say that the value-add of having backtraces
> for them is fairly small compared to the possible impact and that it
> makes sense not to include this in the RFC at all.

I've updated the RFC to v2.0: I'm now proposing an on-off switch to
enable backtraces for fatal errors only.

That said, after updating the RFC I've discussed an alternative with
my colleagues that I think is worth also discussing here. It occurred
to me that the main issue is including the backtrace in
error_get_last, but we want the backtrace in error_get_last as there
is no other way for user code to get the backtrace for a fatal error,
aside from calling error_get_last in shutdown code.

If instead we did call the error handler set up in set_error_handler -
or call something like that - users could gather the backtrace with
debug_backtrace themselves, and log it how they want, or send it
elsewhere, whatever. Internally PHP wouldn't need to do anything about
the backtrace.

Of course users should not be able to actually handle fatal errors,
and execution should immediately bail out after calling the handler as
it does today upon encountering a fatal error. This could be addressed
by either informing users that returning true or false makes no
difference for fatal errors, or by introducing a newer error handler
concept that had that behavior. There may be a backwards compatibility
issue with the former - users might not have written their error
handling code with the expectation that it could encounter fatal
errors - and that too could be addressed by either making this an INI
setting, or again, introducing a newer error handler concept.

Either way, exposing fatal errors to user-level code may be a better
solution to the problem of errors not having a backtrace. Passing
fatal errors to user handler code would potentially make error
handling more consistent, and rely less on global state like
error_get_last. Also note that calling user-level code after a fatal
error is not new; user code registered in register_shutdown_function
is called after a fatal error, which again is one source of a need for
something like error_get_last.

I'm curious if anyone has any immediate reactions - positive or
negative - to this line of thinking. Perfect is the enemy of good, so
I'd accept moving forward with the RFC as it is now if it is more
tenable; I think this idea is worth exploring a little bit before
doing so, however.

Jakub Zelenka

1 year ago

Hi,

On Thu, Dec 5, 2024 at 10:02 PM Eric Norris <eric.t.norris@gmail.com> wrote:

> Hello yet again internals,
>
> I'd like to formally propose a continuation of the original error
> backtraces RFC (https://wiki.php.net/rfc/error_backtraces):
>
> https://wiki.php.net/rfc/error_backtraces_v2
>
>

I would prefer this to be disabled by default (even for fatal errors).
Users need to be aware that they are opting to multi line logging
potentially.

Cheers

Jakub

Jakub Zelenka

1 year ago

On Sun, Dec 8, 2024 at 9:36 AM Jakub Zelenka <bukka@php.net> wrote:

> Hi,
>
> On Thu, Dec 5, 2024 at 10:02 PM Eric Norris <eric.t.norris@gmail.com>
> wrote:
>
>> Hello yet again internals,
>>
>> I'd like to formally propose a continuation of the original error
>> backtraces RFC (https://wiki.php.net/rfc/error_backtraces):
>>
>> https://wiki.php.net/rfc/error_backtraces_v2
>>
>>
>  I would prefer this to be disabled by default (even for fatal errors).
> Users need to be aware that they are opting to multi line logging
> potentially.
>

I know that there's a voting option for that - just saying my preference
for that vote basically.

Eric Norris

1 year ago

Hello,

We're nearing the two-week mark, so I'm curious if anyone has any
other feedback or points of discussion for this RFC.

In particular, I'm interested to know if anyone has thoughts on a
suggestion I had in one of the reply threads - that we could consider
something to the effect of allowing a user-supplied function for fatal
errors - and if that is worth pursuing. If not, I'll continue with the
RFC as-is and plan to put it up for a vote when eligible.

Thanks!

Eric Norris

1 year ago

On Mon, Dec 16, 2024 at 11:17 AM Eric Norris <eric.t.norris@gmail.com> wrote:

>
> Hello,
>
> We're nearing the two-week mark, so I'm curious if anyone has any
> other feedback or points of discussion for this RFC.
>
> In particular, I'm interested to know if anyone has thoughts on a
> suggestion I had in one of the reply threads - that we could consider
> something to the effect of allowing a user-supplied function for fatal
> errors - and if that is worth pursuing. If not, I'll continue with the
> RFC as-is and plan to put it up for a vote when eligible.
>
> Thanks!

I've added a voting choice, "If fatal_error_backtraces defaults to 1,
default value in run-tests.php", in response to discussion in the
GitHub PR.

Claude Pache

1 year ago

> Le 5 déc. 2024 à 21:57, Eric Norris <eric.t.norris@gmail.com> a écrit :
>
> Hello yet again internals,
>
> I'd like to formally propose a continuation of the original error
> backtraces RFC (https://wiki.php.net/rfc/error_backtraces):
>
> https://wiki.php.net/rfc/error_backtraces_v2
>

Hi,

Would be reasonable to be able to recover the stack trace of the error as a string, in addition to the proposed format? In general, it is the format I use in my error/exception handlers for convenience.

A utility function that stringify the trace would be sufficient, as the operation is non-trivial.

—Claude

Unnamed Person

1 year ago

> Claude Pache <claude.pache@gmail.com> hat am 18.12.2024 22:59 CET geschrieben:
>
>
> > Le 5 déc. 2024 à 21:57, Eric Norris <eric.t.norris@gmail.com> a écrit :
> >
> > Hello yet again internals,
> >
> > I'd like to formally propose a continuation of the original error
> > backtraces RFC (https://wiki.php.net/rfc/error_backtraces):
> >
> > https://wiki.php.net/rfc/error_backtraces_v2
> >
>
>
> Hi,
>
> Would be reasonable to be able to recover the stack trace of the error as a string, in addition to the proposed format? In general, it is the format I use in my error/exception handlers for convenience.
>
> A utility function that stringify the trace would be sufficient, as the operation is non-trivial.
>
> —Claude

Some years ago, we had another rfc on this topic:

https://wiki.php.net/rfc/error_handler_callback_parameters_passed_by_reference

Best Regards
Thomas

Eric Norris

1 year ago

> Hi,
>
> Would be reasonable to be able to recover the stack trace of the error as a string, in addition to the proposed format? In general, it is the format I use in my error/exception handlers for convenience.
>
> A utility function that stringify the trace would be sufficient, as the operation is non-trivial.

It is reasonable to provide such a utility function, though I could
imagine some might argue that a function like that is better left to
userland libraries. In any case, I think I would prefer to leave that
for a future RFC in order to keep the scope of this one small.

Eric Norris

1 year ago

On Thu, Dec 5, 2024 at 3:57 PM Eric Norris <eric.t.norris@gmail.com> wrote:

>
> Hello yet again internals,
>
> I'd like to formally propose a continuation of the original error
> backtraces RFC (https://wiki.php.net/rfc/error_backtraces):
>
> https://wiki.php.net/rfc/error_backtraces_v2
>
> We recently had a production incident that might have been shortened,
> had we been able to identify an errant infinite loop in our code from
> the backtrace of a PHP timeout error. It's likely that there are
> plenty of other error conditions that would be easier to debug with a
> stacktrace, too.
>
> The original RFC is inactive, but I've read the proposal and resulting
> discussion and I believe that @nikic's suggestion is a viable way to
> address the RFC's issues. I've implemented that suggestion here:
>
> https://github.com/php/php-src/pull/17056
>
> The RFC currently has two voting choices, the first is to accept an
> INI setting to configure an error mask for backtraces, and the second
> is to choose the default value for the error mask.
>
> Please take a look at let me know what you think.
>
> Thank you!

As it has now been two weeks and discussion has quieted down, I've now
opened the RFC for voting. Once again I've added an additional week
due to the holidays, so this will remain open until 2025-01-10
00:00:00 UTC.

Thank you for the feedback so far.