Always base the timeout duration on the configured Lambda timeout #3

mnapoli · 2021-03-26T15:26:07Z

I'll detail the changes in the diff

mnapoli · 2021-03-26T15:26:44Z

docs/environment/timeouts.md

-application took more time than expected.
-
-In the vast majority of cases, it is an external call to a database, cache or API
-that is stuck waiting for IO.


I've moved this section up and merged it with previous paragraphs

mnapoli · 2021-03-26T15:27:46Z

docs/environment/timeouts.md


 ## Catching the exception

-If you are using a framework, then the framework is probably catching all exceptions
-and displays an error page for the users. You may of course catch the exception
-yourself:


I clarified this to separate php-XX users and php-XX-fpm users, because framework users on FPM could have been confused by the example with the Lambda handler below

src/Timeout/Timeout.php

mnapoli · 2021-03-26T15:30:12Z

tests/Runtime/LambdaRuntimeTest.php

-        $this->assertEquals(5, $getTimeout(LambdaRuntime::fromEnvironmentVariable()));
-        $this->assertEquals(-1, $getTimeout(LambdaRuntime::fromEnvironmentVariable(-1)));
-        $this->assertEquals(0, $getTimeout(LambdaRuntime::fromEnvironmentVariable(0)));
-        $this->assertEquals(10, $getTimeout(LambdaRuntime::fromEnvironmentVariable(10)));


Turned it into a functional test just so that we sleep better at night 😄

mnapoli · 2021-03-26T15:30:57Z

tests/Server.php

+        }
+        if (self::isListening()) {
+            throw new \RuntimeException('Unable to stop node.js server');
+        }


I started having some weird things happen at some point, not related to the PR sorry!

I think I had a leftover process, so I added some more checks here to make sure we successfully kill the test server.

mnapoli · 2021-03-26T15:31:05Z

tests/Server.php

-                . self::$port . ' >> /tmp/server.log 2>&1 &');
-            self::wait();
+        if (self::isListening()) {
+            throw new \Exception('Server is already running');


mnapoli · 2021-03-26T15:31:54Z

website/package-lock.json

@@ -445,7 +445,6 @@
      "integrity": "sha512-5dgndWOriYSm5cnYaJNhalLNDKOqFwyDB/rr1E9ZsGciGvKPs8R2xYGCacuf3z6K1YKDz182fd+fY3cn3pMqXQ==",
      "dev": true,
      "dependencies": {
-        "graceful-fs": "^4.1.6",


Argh sorry, not related. I've been having this line change every time I preview the documentation locally. It finally managed to find its way into a commit, my bad. Let's ignore this.

Nyholm

Thank you. Most looks okey. But there is a nasty bug with the FPM thing.

If you test it, it will work until first timeout, then it will never handle a timeout properly again.

runtime/layers/fpm/bootstrap

Nyholm · 2021-03-26T15:41:11Z

src/Runtime/LambdaRuntime.php

    {
        if ($apiUrl === '') {
            die('At the moment lambdas can only be executed in an Lambda environment');
        }

        $this->apiUrl = $apiUrl;
        $this->invoker = new Invoker;
-        $this->timeout = $timeout;
+        $this->enableTimeout = (getenv('BREF_FEATURE_TIMEOUT') !== false);


I strongly think we should be able to construct a LambdaRuntime without using environment variables. That will make testing easier, allow us to be thread safe and give the creator more control.

I suggest moving this to a named factory method.

Right, I see. TBH my goal is to have simple logic. If we have an internal boolean flag to control the timeout, that means we have 2 different options for the same thing:

the boolean flag

the boolean BREF_FEATURE_TIMEOUT env variable

That makes 4 possible combinations, instead of 2. Not much, but 2 possibilities are simpler to deal with than 4.

What would we gain if we added this little extra complexity? I don't think testing would be impacted, given tests already exist and work. I'm also happy if I can write tests as high level as possible (using the same API as users when possible).

Regarding thread safety, I don't see where multithreading the lambda runtime class would be used in the wild. We could also solve that by using $_SERVER if that ever happens.

So to me it boils down to: there's a single feature flag for the feature instead of 2.

I understand that you would like to have simplicity.

I still think it is a mistake to let a class' internal behaviour to be affected by the global state. I dont follow your logic about the increased combinations. What I suggest is the following:

public static function fromEnvironmentVariable(): self { return new self((string) getenv('AWS_LAMBDA_RUNTIME_API'), getenv('BREF_FEATURE_TIMEOUT') !== false); } public function __construct(string $apiUrl, bool $enableTimeout /* maybe a default value */) { if ($apiUrl === '') { die('At the moment lambdas can only be executed in an Lambda environment'); } $this->apiUrl = $apiUrl; $this->invoker = new Invoker; $this->enableTimeout = $enableTimeout; }

OK I see, with the example indeed it's more concise that what I pictured, thanks.

I tried to go with it, but that got me here:

On one hand, that's cool because I can get rid of changing the global state for the tests, and pass true/false explicitly.

But on the other hand, what I want to test is that the BREF_FEATURE_TIMEOUT setting actually works. So I want to do a very high level test, I consider it definitely ok here to change the environment variable because I would be testing the same API that end users actually use.

FTR I was going to say there's no downside, but the last commit I just pushed will show that it's not true: I have to be careful resetting the global state. But it still feels more comfortable with a higher level test here.

src/Timeout/Timeout.php

…rap process

…, to allow disabling the feature

# Conflicts: # src/Timeout/Timeout.php # tests/Timeout/TimeoutTest.php

Always base the timeout duration on the configured Lambda timeout

6b11d57

mnapoli commented Mar 26, 2021

View reviewed changes

src/Timeout/Timeout.php Outdated Show resolved Hide resolved

mnapoli commented Mar 26, 2021

View reviewed changes

mnapoli mentioned this pull request Mar 26, 2021

Handle timeouts more gracefully by allowing the application to shutdown brefphp/bref#895

Open

2 tasks

Nyholm reviewed Mar 26, 2021

View reviewed changes

mnapoli added 6 commits March 28, 2021 17:55

Make sure that we can never enable the Bref timeout in the FPM bootst…

ce90622

…rap process

Remove unnecessary private method timeoutAfter()

d0e9927

Allow using falsy values for BREF_FEATURE_TIMEOUT, e.g. 0 or ''…

59e6cca

…, to allow disabling the feature

Cleanup global state

c2f7319

Merge branch 'timeouts' into timeouts-feature-flag

53473e6

# Conflicts: # src/Timeout/Timeout.php # tests/Timeout/TimeoutTest.php

Improve tests following @Nyholm's feedback

0b5e9a4

Nyholm merged commit 0b6d7b2 into Nyholm:timeouts Mar 30, 2021

mnapoli deleted the timeouts-feature-flag branch March 30, 2021 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always base the timeout duration on the configured Lambda timeout #3

Always base the timeout duration on the configured Lambda timeout #3

mnapoli commented Mar 26, 2021

mnapoli Mar 26, 2021

mnapoli Mar 26, 2021

mnapoli Mar 26, 2021

mnapoli Mar 26, 2021

mnapoli Mar 26, 2021

mnapoli Mar 26, 2021

Nyholm left a comment

Nyholm Mar 26, 2021

mnapoli Mar 28, 2021

Nyholm Mar 28, 2021 •

edited

Loading

mnapoli Mar 28, 2021 •

edited

Loading

Always base the timeout duration on the configured Lambda timeout #3

Always base the timeout duration on the configured Lambda timeout #3

Conversation

mnapoli commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

mnapoli Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

Nyholm Mar 28, 2021 •

edited

Loading

mnapoli Mar 28, 2021 •

edited

Loading