[HttpKernel] Handle throwable errors in HttpKernel #26514

derrabus · 2018-03-13T17:13:32Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	None
License	MIT
Doc PR	N/A

Currently, HttpKernel only handles exceptions. If the request processing raises an error, that error falls through. In Symfony full-stack applications, this is not a problem, because the framework bundle registers an error handler from the debug component that converts errors to exceptions.

This PR enables HttpKernel to handle errors that have been raised during the request processing, so it can be used standalone in environments where said error handler is not in use.

Test script:

$resolver = new class() implements ControllerResolverInterface
{
    public function getController(Request $request)
    {
        return function () {
            // Raise an error.
            intdiv(42, 0);
        };
    }
};

$dispatcher = new EventDispatcher();
$dispatcher->addListener(KernelEvents::EXCEPTION, function (GetResponseForExceptionEvent $event) {
    $event->setResponse(new Response($event->getException()->getMessage()));
});

$kernel = new HttpKernel($dispatcher, $resolver);

$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$response->send();
$kernel->terminate($request, $response);

Note that this PR does not change how errors are being processed. They still need to be converted before dispatching the kernel.exception event. The only difference is that HttpKernel does not depend on a globally registered error handler anymore to perform that conversion.

nicolas-grekas · 2018-03-14T10:58:10Z

I'm mostly 👎 on this change: the debug component is already a dependency of http-kernel, so you just need to use it. And using the debug component handles real fatal errors, which do still exist. This change could give the illusion that nothing else is needed. That's not true, and that's why the debug component exists.

derrabus · 2018-03-14T13:03:13Z

I'm fine with the debug component being a dependency of the kernel. In fact, this PR makes use of the component as well. Also, I don't say that a proper error handler is not needed. I just don't see why I'm required to use the error handler from the debug component.

Take the script from the PR description: Why am I required to register a specific global error handler to make this script work?

nicolas-grekas · 2018-03-14T13:06:26Z

Why am I required to register a specific global error handler to make this script work?

because you want to catch fatal errors, and this is the only way to do it?

derrabus · 2018-03-14T13:14:47Z

because you want to catch fatal errors, and this is the only way to do it

I could use any other error handler for fatal errors. That's not the point. The script does not raise a fatal error. It raises a simple DivisionByZeroError that is catchable in php 7.

derrabus · 2018-03-14T13:25:35Z

Also, if I changed the controller above to raise an exception instead, that exception would be nicely handled. But an error falls through. That feels inconsistent.

nicolas-grekas · 2018-03-14T13:28:09Z

Exception and Error are very different kind of throwables, that's why PHP core made them separate type hierarchies. The current behavior is inconsistent with PHP IMHO. DivisionByZeroError is the only case where this might be debatable. If you consider all the other *Error, you really don't want to recover from them, but fix you code instead (same for DivisionByZeroError btw...). At least that's the current reasoning.

derrabus · 2018-03-14T13:57:03Z

Exception and Error are very different kind of throwables

The main difference is that exceptions are usually thrown by userland code and errors are raised by php core and extensions.

you really don't want to recover from them, but fix you code instead

You're absolutely right. Same goes for many exceptions btw, like InvalidArgumentException or BadMethodCallException.

What I expect an application framework to do in this case is to fail nicely, provide me with helpful information and (in case it happens on production) give me the chance to log that error somehow. That is to my understanding the purpose of the global try/catch block in HttpKernel. And that block is – imho – incomplete if it ignores errors.

nicolas-grekas · 2018-03-14T13:59:13Z

Yep, same expectations, including for fatal errors on my side. This is already provided by Debug, and thinking catching solves the problem is an illusion :)

derrabus · 2018-03-14T14:24:58Z

same expectations, including for fatal errors on my side.

As you said, that's what we need an error handler for. It was not my intention to solve fatals.

This is already provided by Debug, and thinking catching solves the problem is an illusion :)

So with that argument, we could remove the entire try/catch block from HttpKernel. In fact, the debug component really handles it nicely, if I actually do that. 😮

Yet, that block is there and it serves a purpose. I'm only updating it for the php 7 era.

nicolas-grekas · 2018-03-14T16:08:50Z

From Slack:

derrabus [3:55 PM]
Hey. I feel like we’re spinning circles with the discussion on that PR. I’m really surprised you’re opposing the PR that heavily. If you have some time, I’d like to discuss it with you.

nicolasgrekas [3:57 PM]
Hi 🙂
I feel like blurring the lines between Exception and Error type hierarchies is not a good idea
I prefer clear separate handling paths
an unhandled Error in not different from an unhandled fatal error IMHO
but your pushing to make it the other way around: split fatal errors from Error, and make Error like Exception

derrabus [3:59 PM]
Okay, so we mainly have a different understanding of the Error type.

nicolasgrekas [3:59 PM]
if I reason asymptotically, all fatal errors will be converted to Error by php core
yes 🙂

derrabus [4:01 PM]
So, php documents the class as follows: “Error is the base class for all internal PHP errors.”
It does not say anything about the severity of the error, it simply says: this is an error that has been raised by the php core and not by userland code.

nicolasgrekas [4:02 PM]
the severity is always high, because it's always a programming mistake

derrabus [4:03 PM]
I agree that fatals have to be handled differently. That’s why you cannot catch them.

nicolasgrekas [4:03 PM]
not something anybody shuld silently catch and discard

derrabus [4:03 PM]
I agree, but that’s not what we’re doing, right?

nicolasgrekas [4:03 PM]
actually that's not my understanding of fatal errors: they are just situations that are too hard for php code to turn into Error, due to internal details
(until now)
I really wish nobody catches Error (thus Throwable), and deal with them indifferentialy (unless rethrown) (edited)
I do also understand your POV of course.

derrabus [4:06 PM]
Okay, but what exactly are we doing differently then? Right now, the error is converted into a FatalThrowableError exception and pushed through the application’s kernel.exception event, right?

nicolasgrekas [4:06 PM]
via terminateWithException yes
another global exception handler could see them (edited)

derrabus [4:08 PM]
So, the only difference with my PR would be that the kernel itself would trigger that event and not the error handler?

nicolasgrekas [4:08 PM]
if you look at handleException, it does some things
like setting exitCode to 255, converting the Error to Exception (thus no need for FlattenedThrowable), and logs early on (edited)

derrabus [4:17 PM]
Does the exit code have any effect in a web request? Exceptions are logged, so we would log errors twice right now.

nicolasgrekas [4:18 PM]
that's some sort of failsafe I think
if the complex logic doesn't work, at least the lowlevel one in handleException might have

derrabus [4:20 PM]
Also, I don’t want to remove the error handler. We still need it for fatals and everything that could go wrong before or after request processing.

nicolasgrekas [4:20 PM]
ok, so what's the use case ?

derrabus [4:23 PM]
Okay, mainly for teaching purposes, I was trying to use some Symfony components outside of the context of a full-stack application, to show how the components play together. I found the fact that HttpKernel enables me to fail nicely on exceptions but not on erros really confusing.

nicolasgrekas [4:24 PM]
OK, so not something that justifies the change IMHO
"just" teach the next level: fatal error handling 🙂

derrabus [4:25 PM]
Yeah, probably. Unless somebody would try to build the next Silex or something, the current way does not hurt anybody.

nicolasgrekas [4:26 PM]
yep, and they would still need Debug to deal with fatal errors 🙂

derrabus [4:28 PM]
Yes. But with my change applied, they could use any other error handler. The odd thing about the current solution is, that only the error handler from Debug works properly on errors because it does that odd pusing the error through the event dispatcher thingy.

nicolasgrekas [4:29 PM]
that's a feature of the Debug error handler 🙂
any other can still handle Error and Fatal Error the way they want

derrabus [4:32 PM]
Yes, but they have to handle errors because the kernel does not catch them despite $catch = true. They could solve that by extending the HttpKernel class, yet it feels unnecessarily complicated. (edited)

nicolasgrekas [4:32 PM]
but they'd also have to deal with fatal errors if they want to build a real framework
so back to the current situtation

derrabus [4:34 PM]
Again, that’s “we need to use any proper error handler” Vs. “we need to use the error handler from Debug“.

nicolasgrekas [4:35 PM]
why?
why any other one couldn't do it ?
my pov is that fatals and Error should reunited (edited)
vs Exception + Error
so any alternative error handler would just deal with fatals and uncaught errors (edited)

derrabus [4:39 PM]
Right, from that PoV the current way is understandable. And since I probably won’t convince you from my PoV that errors are just exceptions thrown by the php interpreter, my PR probably does not have a chance to go through. 😕

nicolasgrekas [4:40 PM]
sorry about that 🙂 error handling is a sensitive and complex topic
once it's stable, I'm very reluctant to change it
the current way proved stable, and removed many if not all blank pages
I'm conservative here, I know 🙂 that doesn't make you wrong... (edited)
(but my way it more "proved")

derrabus [4:43 PM]
Right. I don’t think that my change would raise the danger of blank pages though, since Debug’s error handler would still serve as a safety net as it does now.

nicolasgrekas [4:43 PM]
but it might raise the danger of not logged messages 🙂

derrabus [4:43 PM]
But okay, now I know your reasoning behind it. Thanks for taking the time. 😃

nicolasgrekas [4:44 PM]
thanks to you 🙂
I'm happy to share these, now I'm not alone knowing why this current design makes sense
hopefully 🙂

derrabus [4:47 PM]
I will stop tinkering with the error handling then. If there’s anything else I could help with before the feature freeze, let me know. 😉

…to show `HttpKernel::handle()` will catch throwables (Nyholm) This PR was merged into the 6.2 branch. Discussion ---------- [FrameworkBundle][HttpKernel] Add deprecation warning to show `HttpKernel::handle()` will catch throwables | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | no | Deprecations? | yes | Tickets | Fix #16205 | License | MIT | Doc PR | symfony/symfony-docs#...  I suggest that starting from Symfony 7.0 the `HttpKernel` will start to catch `\Throwable` and convert them to a `Response`. This was first asked in #16205, I face a similar issue with Runtime component and Bref.. ---------- The reason I push for this change is to embrace the request/response workflow of the Kernel without trusting the custom error handler. In an environment where you serve multiple requests with the same PHP process (read: RoadRunner, Swoole, Bref) you would write something like: ```php $kernel = new Kernel('prod', false); while (true) { $request = /* create sf request from custom environment */ try { $response = $kernel->handle(request); return ResponseConverter::convert($response); } catch (\Throwable $e) { exit(1); } } ``` (pseudo code of course. Here is a [real example](https://github.com/php-runtime/bref/blob/0.3.1/src/BrefRunner.php#L30-L43)) The `exit(1)` means a hard crash. For Bref Runtime it would result in a 500 error from API-gateway. Since the `\Throwable` is caught, the Symfony error handler is not used. If we would not to catch the `\Throwable`, then the Symfony error handler would be used, but it would print a Response instead of returning it. (Printing a response will just add HTML on the CLI...) ----------- Other PRs and issues related to this: - #45301 - #26514 - #22128 - #36885 - #25467 I'm happy to let the `HttpKernel` to catch the `\Throwable` exception right now, but I thought this very conservative PR would have a higher change to get merged. Also note that we do not specify any behaviour on our [HttpKernelInterface](https://github.com/symfony/symfony/blob/v6.0.7/src/Symfony/Component/HttpKernel/HttpKernelInterface.php#L43) ------- To remove the deprecation message you need to add this to your config: ```yaml framework: catch_all_throwables: true ``` Commits ------- 7977a15 Add deprecation warning to show HttpKernel::handle() will catch throwables

carsonbot added Status: Needs Review Feature labels Mar 13, 2018

Handle errors in HttpKernel.

0a8215d

derrabus force-pushed the throwable-in-http-kernel branch from 203804d to 0a8215d Compare March 13, 2018 17:20

derrabus changed the title ~~Handle errors in HttpKernel~~ [HttpKernel] Handle throwable errors in HttpKernel Mar 13, 2018

nicolas-grekas added this to the 4.1 milestone Mar 14, 2018

nicolas-grekas closed this Mar 14, 2018

derrabus mentioned this pull request Jul 4, 2019

[HttpKernel] Support for Throwable #22128

Closed

derrabus mentioned this pull request Aug 8, 2019

Cleanup the new ErrorHandler component #32605

Closed

5 tasks

derrabus mentioned this pull request Oct 31, 2019

Deprecate things that prevent \Throwable from bubbling down #33065

Merged

derrabus mentioned this pull request May 25, 2020

Update HttpKernel.php #36885

Closed

derrabus mentioned this pull request Feb 3, 2022

[HttpKernel] Error handling #45301

Closed

Nyholm mentioned this pull request Apr 11, 2022

[FrameworkBundle][HttpKernel] Add deprecation warning to show HttpKernel::handle() will catch throwables #45997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[HttpKernel] Handle throwable errors in HttpKernel #26514

[HttpKernel] Handle throwable errors in HttpKernel #26514

Uh oh!

derrabus commented Mar 13, 2018 •

edited

Loading

Uh oh!

nicolas-grekas commented Mar 14, 2018

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

nicolas-grekas commented Mar 14, 2018

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

nicolas-grekas commented Mar 14, 2018

Uh oh!

Uh oh!

Uh oh!

[HttpKernel] Handle throwable errors in HttpKernel #26514

[HttpKernel] Handle throwable errors in HttpKernel #26514

Uh oh!

Conversation

derrabus commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Mar 14, 2018

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

nicolas-grekas commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

nicolas-grekas commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

nicolas-grekas commented Mar 14, 2018

Uh oh!

derrabus commented Mar 14, 2018

Uh oh!

nicolas-grekas commented Mar 14, 2018

Uh oh!

Uh oh!

derrabus commented Mar 13, 2018 •

edited

Loading

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

nicolas-grekas commented Mar 14, 2018 •

edited

Loading