Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Return non-zero exit code for uncaught exceptions #46541

Merged
merged 12 commits into from
Apr 6, 2023

Conversation

bert-w
Copy link
Contributor

@bert-w bert-w commented Mar 21, 2023

This is a fix for #46306.

Any uncaught exceptions inside the exception handler now explicity exit with code 1, indicating an error state. A test has been included to test for the correct exit code in case of an uncaught exception and in the case of successful execution.

EDIT: thanks to Crynobone for cleaning up the testcase to work neatly with the testbench

@bert-w bert-w marked this pull request as ready for review March 21, 2023 22:19
@taylorotwell
Copy link
Member

Honestly for a simple change I would just leave all tests unchanged. Just creates more maintenance overhead than it's worth.

@taylorotwell taylorotwell marked this pull request as draft March 25, 2023 11:59
}

if (static::$app->runningInConsole()) {
$this->renderForConsole($e);
} else {
$this->renderHttpResponse($e);
}

if ($uncaught) {
exit(1);
Copy link
Member

@crynobone crynobone Mar 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this cause Octane to exit the worker on every uncaught exception, which is not desired?

laravel/octane#654

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question. That could be the case yes.

Anyhow, there is no way to say to php "exit with exitCode=1 on your own accord (when its time to exit)". AFAIK you must call exit(1) yourself which will then still call any remaining __destruct() listeners and shutdown handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the exit(1) call a few lines up now (inside the $app->runningInConsole() since this is a command line related issue)

Copy link
Member

@crynobone crynobone Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Octane workers still will have $app->runningInConsole() === true since it is an artisan command.

Octane worker emits APP_RUNNING_IN_CONSOLE=false so should be okay.

$this->isRunningInConsole = Env::get('APP_RUNNING_IN_CONSOLE') ?? (\PHP_SAPI === 'cli' || \PHP_SAPI === 'phpdbg');

@bert-w
Copy link
Contributor Author

bert-w commented Mar 25, 2023

@taylorotwell I prefer to leave the test in since the behaviour does need to be tested. If you have a faster/cleaner way to test this with less code then by all means :)

One way or another you need to call an external process with a php artisan ... command; the trait MakesArtisanScript is the most straightforward solution to have the correct boilerplate in the test(s).

@bert-w
Copy link
Contributor Author

bert-w commented Mar 30, 2023

@crynobone do you have any idea on how to rewrite the testcase using the testbench such that I can test the exitcode for running an artisan command?

@crynobone
Copy link
Member

@bert-w Can you share an artisan command example?

@bert-w
Copy link
Contributor Author

bert-w commented Mar 30, 2023

@crynobone in essence doing the following:

/** @var \Illuminate\Foundation\Console\Kernel $kernel */
$exitCode = $kernel->call('throw-exception-command');

However since the command is meant to throw and I want to test the exitcode, this call must happen as a separate process or else the testcase itself will throw the exception.

This PR currently includes a wrapper to create an explicit artisan file for inside the Testbench but it is a bit convoluted that way.

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone
Copy link
Member

@bert-w See bert-w#1

Use `PhpProcess` to run PHP script in isolation to verify `exit()`
@bert-w bert-w marked this pull request as ready for review March 30, 2023 19:54
@taylorotwell
Copy link
Member

Is there any way an application could enter this section of code without being in an unrecoverable / fatal error state? In other words, is this going to break any applications that could have possible kept executing code after this handleException method is called?

@bert-w
Copy link
Contributor Author

bert-w commented Apr 6, 2023

I don't see how that could be possible since that function is set using set_exception_handler(...). This handleException function thus is only executed on an uncaught exception. PHP says "Execution will stop after the callback is called." (see https://www.php.net/manual/en/function.set-exception-handler.php)

Then once exit(1) is called, only shutdown handlers register_shutdown_function(...) and destructors __destruct() are called (see https://www.php.net/manual/en/function.exit.php).

The only destructor being called after this in a default Laravel app is the Monolog handler itself, but whether it is called because of an exit() or because of an object dereference shouldnt make a difference.

@taylorotwell taylorotwell merged commit 6114330 into laravel:9.x Apr 6, 2023
@bert-w bert-w deleted the return-non-zero-exit-codes branch April 25, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants