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

Finish request early: frankenphp_finish_request #69

Merged
merged 9 commits into from
Nov 3, 2022

Conversation

withinboredom
Copy link
Collaborator

@withinboredom withinboredom commented Oct 26, 2022

This adds a new function: frankenphp_finish_request and an alias for it: fastcgi_finish_request, which follows in the footsteps of other SAPIs.

closes #62

@withinboredom withinboredom changed the title [WIP] Finish request early [WIP] Finish request early: frankenphp_finish_request Oct 26, 2022
frankenphp.stub.php Outdated Show resolved Hide resolved
frankenphp_arginfo.h Show resolved Hide resolved
testdata/finish-request.php Outdated Show resolved Hide resolved
frankenphp.c Show resolved Hide resolved
frankenphp.c Show resolved Hide resolved
frankenphp.go Outdated Show resolved Hide resolved
frankenphp_test.go Outdated Show resolved Hide resolved
@withinboredom withinboredom marked this pull request as ready for review October 31, 2022 09:41
@withinboredom withinboredom changed the title [WIP] Finish request early: frankenphp_finish_request Finish request early: frankenphp_finish_request Oct 31, 2022
@chalasr
Copy link
Contributor

chalasr commented Oct 31, 2022

Is the fastcgi_finish_request alias really needed? What does it bring?

@withinboredom
Copy link
Collaborator Author

Is the fastcgi_finish_request alias really needed? What does it bring?

Mostly it brings compatibility with other scripts/libraries without needing to make specific changes just to support frankenphp.

@chalasr
Copy link
Contributor

chalasr commented Oct 31, 2022

Other SAPIs don't have such alias though (or even removed it for some reason)

@dunglas
Copy link
Owner

dunglas commented Nov 1, 2022

NGINX Unit implements this function (with no alias): https://github.com/nginx/unit/blob/master/src/nxt_php_sapi.c#L216

Having it will improve the performance of the hundreds of applications out there relying on this feature.

@dunglas
Copy link
Owner

dunglas commented Nov 1, 2022

And according to https://github.com/nginx/unit/issues?q=is%3Aissue+fastcgi_finish_request (no bug report since this feature has been implemented), it's quite safe to do as long as this function behaves exactly as the original one (which should be the case here).

Also, if someone really wants to exclude FrankenPHP, it's quite easy to do using feature/SAPI detection.

@chalasr
Copy link
Contributor

chalasr commented Nov 1, 2022

Makes sense. Thank you guys for the explanations

@dunglas dunglas merged commit 9ef3bd7 into dunglas:main Nov 3, 2022
@withinboredom withinboredom deleted the finish-request-early branch November 3, 2022 21:52
@withinboredom
Copy link
Collaborator Author

🎉

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

Successfully merging this pull request may close these issues.

Consider implementing fastcgi_finish_request()?
3 participants