Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126117
Precedence: bulk
MIME-Version: 1.0
References: <CA+NMNO1zgCLLDOaWhqg8ipOrLETRn3w_91JNdCJz0zrKmeRWyg@mail.gmail.com>
 <8f268a8e-1646-455e-a1c2-b030581dcb9f@bastelstu.be>
In-Reply-To: <8f268a8e-1646-455e-a1c2-b030581dcb9f@bastelstu.be>
Reply-To: erictnorris@gmail.com
Date: Thu, 5 Dec 2024 20:08:27 -0500
Message-ID: <CA+NMNO1LOXwLiRs51p-QnVkcs829RHxpZkC=ZArhFUTz6Qg-aA@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] [Discussion] Error backtraces v2
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: eric.t.norris@gmail.com (Eric Norris)

Hello,

On Thu, Dec 5, 2024 at 5:31=E2=80=AFPM Tim D=C3=BCsterhus <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 =E2=80=9Cphp.ini Defaults=E2=80=9D section should probably refer to t=
he 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?