-
-
Notifications
You must be signed in to change notification settings - Fork 374
Flush FPM logs in case of timeouts #1133
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Bref\Event\Http\FastCgi; | ||
|
|
||
| /** | ||
| * There was a timeout while processing the PHP request | ||
| * | ||
| * @internal | ||
| */ | ||
| final class Timeout extends \Exception | ||
| { | ||
| public function __construct(int $taskTimeoutInMs) | ||
| { | ||
| $message = "The request timed out after $taskTimeoutInMs ms. " | ||
| . 'Note: that duration may be lower than the Lambda timeout, don\'t be surprised, that is intentional. ' | ||
| . 'Indeed, Bref stops the PHP-FPM request *before* a hard Lambda timeout, because a hard timeout prevents all logs to be written to CloudWatch.'; | ||
|
|
||
| parent::__construct($message); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |||||
| namespace Bref\Test\Handler; | ||||||
|
|
||||||
| use Bref\Context\Context; | ||||||
| use Bref\Event\Http\FastCgi\FastCgiCommunicationFailed; | ||||||
| use Bref\Event\Http\FastCgi\Timeout; | ||||||
| use Bref\Event\Http\FpmHandler; | ||||||
| use Bref\Test\HttpRequestProxyTest; | ||||||
| use DMS\PHPUnitExtensions\ArraySubset\ArraySubsetAsserts; | ||||||
|
|
@@ -1071,7 +1071,7 @@ public function test FPM timeouts are recovered from() | |||||
| 'httpMethod' => 'GET', | ||||||
| ], $this->fakeContext); | ||||||
| $this->fail('No exception was thrown'); | ||||||
| } catch (FastCgiCommunicationFailed $e) { | ||||||
| } catch (Timeout $e) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be 👇 ?
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this test I specifically want to test for a timeout, so if I get any other exception it's that the test is broken? (and so the |
||||||
| // PHP-FPM should work after that | ||||||
| $statusCode = $this->fpm->handle([ | ||||||
| 'version' => '1.0', | ||||||
|
|
@@ -1084,6 +1084,26 @@ public function test FPM timeouts are recovered from() | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * See https://github.com/brefphp/bref/issues/862 | ||||||
| */ | ||||||
| public function test worker logs are still written in case of a timeout() | ||||||
| { | ||||||
| $this->fpm = new FpmHandler(__DIR__ . '/PhpFpm/timeout.php', __DIR__ . '/PhpFpm/php-fpm.conf'); | ||||||
| $this->fpm->start(); | ||||||
|
|
||||||
| try { | ||||||
| $this->fpm->handle([ | ||||||
| 'version' => '1.0', | ||||||
| 'httpMethod' => 'GET', | ||||||
| ], new Context('abc', time(), 'abc', 'abc')); | ||||||
| $this->fail('No exception was thrown'); | ||||||
| } catch (Timeout $e) { | ||||||
| $logs = ob_get_contents(); | ||||||
| self::assertStringContainsString('This is a log message', $logs); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @see https://github.com/brefphp/bref/issues/316 | ||||||
| */ | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| error_log('This is a log message'); | ||
|
|
||
| sleep((int) ($_GET['timeout'] ?? 10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnapoli was this merged into 1.5.0? Seems like extraneous debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is demo code, nothing to worry about IMHO
It just allows to test this feature works but will never be executed in you app 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this directory isn't even a demo really, just a sample app for me to play/develop. It's not shipped to users.