Skip to content

Control FPM timeout to give time to flush out logs #1106

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.

Sign up for GitHub

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

Closed
deleugpn wants to merge 1 commit into from
Closed

Control FPM timeout to give time to flush out logs #1106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,25 @@ class ServerlessPlugin {
if (f.runtime === 'provided') {
throw new this.serverless.classes.Error(`Bref 1.0 layers are not compatible with the "provided" runtime.\nTo upgrade to Bref 1.0, you have to switch to "provided.al2" in serverless.yml for the function "${name}".\nMore details here: https://bref.sh/docs/news/01-bref-1.0.html#amazon-linux-2`);
}

this.defineBrefFpmTimeout(name)
}
}

defineBrefFpmTimeout(name) {
const timeout = this.serverless.service.functions[name].timeout ??
this.serverless.service.provider?.environment?.timeout ??
29 // We assume API Gateway Limit.
Copy link
Member

@t-richard t-richard Nov 29, 2021

Choose a reason for hiding this comment

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

Shouldn't the default be 6 seconds (to be confirmed) , the default timeout for functions in the serverless framework?

If a timeout is not explicitly defined, sls would deploy it with a timeout of 6 seconds and fpm would timeout at 28 seconds. This would defeat the purpose of this PR.

Copy link
Member Author

@deleugpn deleugpn Dec 5, 2021

Choose a reason for hiding this comment

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

It's a tricky thing. If we go with 6 seconds here, anybody deploying without Serverless will suddenly see a huge timeout drop in their API Gateway. I know that if they're using API gateway they cannot have it bigger than 30 seconds. For ALB, the use case is much more limited and much more likely to be used by people aware of how AWS works.
Choosing a large amount because of API Gateway will cover the most common case and anybody having issues with 6 second timeout can always increase it to 30 seconds.


const environment = this.serverless.service.functions[name].environment ?? {}

// Here we subtract one second so that FPM can timeout before lambda crashes
// That will give the Lambda execution enough time to flush out logs.
environment.BREF_FPM_TIMEOUT = (timeout - 1) * 1000

this.serverless.service.functions[name].environment = environment;
}

addCustomIamRoleForVendorArchiveDownload() {
this.serverless.service.custom = this.serverless.service.custom ? this.serverless.service.custom : {};
this.serverless.service.custom.bref = this.serverless.service.custom.bref ? this.serverless.service.custom.bref : {};
Expand Down
5 changes: 5 additions & 0 deletions runtime/layers/fpm/bootstrap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ if (! is_file($handlerFile)) {
$lambdaRuntime->failInitialization("Handler `$handlerFile` doesn't exist");
}

$timeout = getenv('BREF_FPM_TIMEOUT');

$phpFpm = new FpmHandler($handlerFile);

$phpFpm->setTimeout((int) $timeout);
Copy link
Member

@t-richard t-richard Nov 29, 2021

Choose a reason for hiding this comment

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

Shouldn't this be called only when a timeout env Var is provided?

Because the variable is set by the serverless framework plugin, it may not be set when deploying with sam/cdk/cloudformation


try {
$phpFpm->start();
} catch (\Throwable $e) {
Expand Down
9 changes: 8 additions & 1 deletion src/Event/Http/FpmHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,20 @@ final class FpmHandler extends HttpHandler
private $configFile;
/** @var Process|null */
private $fpm;
/** @var int */
private $timeout = 900000;

public function __construct(string $handler, string $configFile = self::CONFIG)
{
$this->handler = $handler;
$this->configFile = $configFile;
}

public function setTimeout(int $timeout)
{
$this->timeout = $timeout;
}

/**
* Start the PHP-FPM process.
*/
Expand Down Expand Up @@ -83,7 +90,7 @@ public function start(): void
});

$this->client = new Client;
$this->connection = new UnixDomainSocket(self::SOCKET, 1000, 900000);
$this->connection = new UnixDomainSocket(self::SOCKET, 1000, $this->timeout);

$this->waitUntilReady();
}
Expand Down