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

reverseproxy: Reset Content-Length to prevent fastcgi from hanging #5435

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

trea
Copy link
Contributor

@trea trea commented Mar 15, 2023

…PHP-FPM requests from hanging

See: #5420

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2023

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie changed the title fix(reverse-proxy): Reset Content-Length when the request to prevent … reverseproxy: Reset Content-Length to prevent fastcgi from hanging Mar 15, 2023
@francislavoie francislavoie requested a review from mholt March 15, 2023 18:54
@francislavoie francislavoie added the bug 🐞 Something isn't working label Mar 15, 2023
@francislavoie francislavoie added this to the v2.7.0 milestone Mar 15, 2023
@mholt
Copy link
Member

mholt commented Mar 15, 2023

Awesome @trea -- thank you for the PR!

Since this patch essentially reverts a reversion, can you help us verify that all the following issues are not a problem with this patch:

I have my hands full today so it might be fastest if you are able to confirm these -- but anyone else is welcome to test as well!

@trea
Copy link
Contributor Author

trea commented Mar 15, 2023

Sure thing!

#5236 and #5420 should be fixed by this
#5366 should remain fixed by this because this patch still requires buffering to be enabled

But I do have more related thoughts I'm about to post in #5420. The TL;DR being that I think Caddy may need to implement buffering in its FastCGI transport for reverse proxying.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

(Left a comment in the issue as well.)

I think this is starting to converge on the right solution -- and is a step forward, now that I fixed the OOM bug that I introduced 😅

I like your idea that probably the FastCGI transport should buffer anyway, so it doesn't have to be explicitly enabled, but this is at least a step back in the right direction.

@mholt mholt merged commit 2182270 into caddyserver:master Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants