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

Let LambdaRuntime::processNextEvent() return boolean #1013

Merged
merged 1 commit into from
Sep 19, 2021

Conversation

Nyholm
Copy link
Contributor

@Nyholm Nyholm commented Sep 5, 2021

I would like to know if a handler succeeds or not. Since the LambdaRuntime is echo out data on error, we might run into issues when keeping the process alive for multiple requests.

See https:/php-runtime/bref/blob/0.1.1/src/BrefRunner.php#L34

A simple fix would be to stop the while(true) on error.

Copy link
Member

@t-richard t-richard left a comment

Choose a reason for hiding this comment

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

Looks good to me. Since this class is internal and we're moving from void to bool, it should not have an impact.

@deleugpn
Copy link
Member

deleugpn commented Sep 6, 2021

I think it would make a better API if we could let the exception bubble up. Returning false indicates that it failed but it says nothing about why it failed, would users stop the while(true) for any exception? Some subset of exceptions? We don't know. Since the only available place to know the cause of error would be CloudWatch, it seems like a fragile API.

I'm not sure about an implementation of a secondary parameter where we decide to rethrow the exception or if a separate method would be better. I can see it's a tricky implementation because a separate method would still need try catch for the signalFailure.

@mnapoli
Copy link
Member

mnapoli commented Sep 6, 2021

+1 I'd be curious to know more about what would happen next.

Because this class is internal at the moment, if we make it public API then we need to have a strong & defined use case for that right? (else you'd have no BC promise)

@Nyholm
Copy link
Contributor Author

Nyholm commented Sep 6, 2021

+1 I'd be curious to know more about what would happen next.

Haha. I thought I could sneak in this change first before I made an argument for creating a public API.

Because creating a public API requires some more thinking and discussions. But yes, I would like to make this class non-internal at some point in the future.

mnapoli added a commit that referenced this pull request Sep 19, 2021
This is a continuation of #1013.

In the function runtime, the PHP process will now always be restarted if the invocation failed. That change only impacts apps that use `BREF_LOOP_MAX` to keep the process alive.

The reason for this is that, in most cases, a failed invocation means the PHP application threw an exception.

When using `BREF_LOOP_MAX`, the PHP process is kept alive, and it is important for the application to cleanup global state.

If an exception was thrown, it is likely the cleanup wasn't done, or wasn't done properly. It sounds much safer to restart the process.

The downside is a slight performance cost: around 10ms to restart the process. But that provides a safer execution environment.
@mnapoli
Copy link
Member

mnapoli commented Sep 19, 2021

Following discussions on Slack, we realized there is a sense to add this to Bref because we may want to use that feature to improve the "on-error" behavior.

Follow-up PR: #1026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants