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.

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

Jump to bottom

fix: properly close context on worker script timeouts and crashes. #1184

Merged
merged 2 commits into from
Nov 23, 2024
Merged

fix: properly close context on worker script timeouts and crashes. #1184

merged 2 commits into from
Nov 23, 2024

Conversation

AlliBalliBaba
Copy link
Collaborator

I noticed that PHP timeouts will fully stop the script's execution and not just continue to the next iteration in worker mode.

This PR fixes this issue in part by making sure the context gets closed if a worker request is still pending and the PHP script returned a non-zero status code.

To reproduce, just set max_execution_time = 2 in the php.ini and execute a worker script with sleep(3);. Ideally, timeouts would not stop the script execution (if that's even possible).

@withinboredom
Copy link
Collaborator

Hmmm... I think there must be an issue somewhere. In frankenphp.c it should set the timeout to 0.

@AlliBalliBaba
Copy link
Collaborator Author

It gets unset while waiting, but reset during request execution

@AlliBalliBaba
Copy link
Collaborator Author

The issue is, if it times out during execution, it will exit from the whole script, not just the current frankenphp_handle_request() execution.

@withinboredom
Copy link
Collaborator

I don't think I understand. Wouldn't this be the same as doing an exit or die during a request? Is that what you mean? Or do you mean in the worker script itself?

@dunglas
Copy link
Owner

dunglas commented Nov 21, 2024

This was intended. I'm not sure that it's the best behavior, but it looked more secure to me, especially because workers are restarted anyway.

@AlliBalliBaba
Copy link
Collaborator Author

@withinboredom on timeout it will not behave the same as on exit. On timeout it will stop the whole script and execution is continued after php_execute_script. This PR makes sure that the pending worker request gets properly closed before the worker restarts (so fc.done gets called and the client receives a response).

I'm not sure if the behavior was always this way, but it behaves this way on the newest PHP 8.3 version.

@dunglas
Copy link
Owner

dunglas commented Nov 23, 2024

Ok I think I get it. This looks like a good fix. WDYT @withinboredom ?

@dunglas dunglas merged commit ccf2af7 into main Nov 23, 2024
46 checks passed
@dunglas dunglas deleted the fix/worker-timeouts branch November 23, 2024 19:45
@dunglas
Copy link
Owner

dunglas commented Nov 23, 2024

Thank!!

@AlliBalliBaba AlliBalliBaba mentioned this pull request Nov 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@withinboredom withinboredom withinboredom approved these changes

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlliBalliBaba @withinboredom @dunglas @Alliballibaba2