Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:110337
MIME-Version: 1.0
References: <CAGgaK7KScGnNaCSnz6szCzJ3jndoipH0=yonW6FPKR7GbkPdLw@mail.gmail.com>
 <CA+kxMuRVwhMjZxA0ctz4PK9VNvNTYSga6bg1V54KeqHXMguQgw@mail.gmail.com>
In-Reply-To: <CA+kxMuRVwhMjZxA0ctz4PK9VNvNTYSga6bg1V54KeqHXMguQgw@mail.gmail.com>
Date: Tue, 2 Jun 2020 23:56:57 +0300
Message-ID: <CAGgaK7JJO8bit-dADUpP013PgiyvKzd6Jy6anemwzTkx4UpiUA@mail.gmail.com>
Cc: Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000a6d75605a7202727"
Subject: Re: [PHP-DEV] RFC: Error backtraces
From: maxsem.wiki@gmail.com (Max Semenik)

--000000000000a6d75605a7202727
Content-Type: text/plain; charset="UTF-8"

Thanks for your input!

On Sat, May 30, 2020 at 8:36 PM Dan Ackroyd <Danack@basereality.com> wrote:

> For my own code, I convert all non-silenced errors and warnings into
> an exception through the error handler*. That works pretty well, but
> has one big downside - it only throws one type of exception.
>
> I wrote some words about what we could do:
> https://gist.github.com/Danack/6b5a86414ab9e9d9ac7e0a906216f1c5
>
> The TL:DR of that is, we could add specific exceptions that should be
> thrown if the error handler setup by the user decided to throw an
> exception. I would be interested in hearing if that idea achieves the
> benefits you're trying to get, in a less controversial way.
>

Your solution requires programmers to write code supporting this kind of
workflow, while I'm interested in providing useful information in all
cases, even for legacy apps.


> This has security implications as it would be possible for app secrets
> to be included in the error message. That is less of a problem for
> exceptions as you can write your own code for logging exceptions that
> doesn't expose the variables if you don't want them exposed.
>

That's why I propose an INI setting controlling this behavior.


> "..., traces are always generated for exceptions"


> That depends on what callback function has been set for
> 'set_exception_handler'.


From my reading of the code, backtraces are generated on object creation,
in zend_default_exception_new_ex().


> Also....maybe use more formal language for
> RFCs. Even minor swear words should be avoided imo.
>

Sorry, didn't realize the word is, uh, "informal".


> "error_get_last() output will have another element added to it if a
> trace is available:"
>
> I strongly suspect that is a very bad idea. As the variables in the
> stack trace are kept alive while that stack trace is kept alive, that
> means the lifetime of those variables would depend on how long until
> another error occurs.
>

As shown in the example output, traces are saved as string, so no
implications to lifetime.


> "In the current proposed implementation, a new error flag
> E_UNHANDLED_EXCEPTION is introduced,"
>
> The RFC doesn't say what this is meant to do precisely.....but my gut
> instinct doesn't like it.


Clarified this with a link to the code: I needed it to avoid outputting the
trace twice in some cases.


> * an error handler that converts all unsilenced errors that we care
> about into an exception
>
> function standardErrorHandler($errorNumber, $errorMessage, $errorFile,
> $errorLine): bool
> [snip]


The problem with this is that it doesn't get called on fatal errors, i.e.
you can't get traces for the most serious problems.

On Sun, May 31, 2020 at 4:47 PM Nikita Popov <nikita.ppv@gmail.com> wrote:

> I'm concerned about the performance implications of this change. Backtrace
> gathering is quite expensive and doing this for every diagnostic will have
> a large performance impact if there are many of them. This is okay if your
> code is intended to be diagnostics clean, but unfortunately this is not a
> given. If you generate 10k deprecation warnings during a request, you're
> definitely going to feel those backtraces.
>

INI setting should take care of this. If your code is crap, don't flip it
on.


> It is already possible to get a backtrace for all non-fatal diagnostics
> through a custom error handler, which allows you to control whether you
> want to collect a backtrace for a particular error. As such, I would
> recommend to limit this functionality to fatal errors only, where such a
> possibility does not currently exist. Additionally fatal errors also
> naturally limit the performance impact, because there can (approximately)
> be only one fatal error per request.
>

I guess having this only for fatals is better than nothing, however since
this functionality is optional, I don't see leaks via traces as a serious
problem. Backtraces are disabled by default in code and php.ini-production,
so it's safe by default.

On Tue, Jun 2, 2020 at 1:29 PM Peter Stalman <sarkedev@gmail.com> wrote:

> Isn't this something that is handled fairly well by xdebug already?


Yes, but most people don't run xdebug in production. Also, it's not part of
PHP core and thus some people might not have it in their hosting
environment. Having basic diagnostics seems to me like a good thing to have
in PHP proper.

-------
I've updated the RFC: clarified some parts and removed the voting question
whether the configuration setting is needed, discussion clearly shows so.


-- 
Best regards,
Max Semenik

--000000000000a6d75605a7202727--