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

Buffer chunked requests #43047

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Jan 23, 2024

Clients like xDavv5 on Android, or Cyberduck, use chunked requests.
When FastCGI or FPM is used with apache, requests arrive to Nextcloud without any content.
This leads to the creation of empty files.
The following directive will force the problematic requests to be buffered before being forwarded to Nextcloud.
This way, the "Transfer-Encoding" header is removed, the "Content-Length" header is set, and the request content is proxied to Nextcloud.
Here are more information about the issue:

Tested for a few weeks by a customer without any issue.

@artonge artonge self-assigned this Jan 23, 2024
@szaimen
Copy link
Contributor

szaimen commented Jan 23, 2024

@artonge artonge force-pushed the artonge/feat/buffer_chunked_requests branch from 3e8cd1d to be94125 Compare January 23, 2024 10:06
@artonge artonge requested review from a team, ArtificialOwl, icewind1991 and nfebe and removed request for a team January 23, 2024 10:06
@artonge artonge added enhancement 3. to review Waiting for reviews labels Jan 23, 2024
@artonge artonge added this to the Nextcloud 29 milestone Jan 23, 2024
@artonge
Copy link
Contributor Author

artonge commented Jan 23, 2024

/backport to stable28

@artonge
Copy link
Contributor Author

artonge commented Jan 23, 2024

/backport to stable27

@artonge
Copy link
Contributor Author

artonge commented Jan 23, 2024

/backport to stable26

@artonge
Copy link
Contributor Author

artonge commented Jan 23, 2024

I guess I can remove this from aio then again? :)

I guess yes. :) The current PR is also narrower, so only changes the behavior for problematic requests.

@SystemKeeper
Copy link
Contributor

SystemKeeper commented Jan 23, 2024

Would this also need to be added to https://github.com/nextcloud/server/blob/master/lib/private/Setup.php#L506 ?
Ah misunderstood the code, sorry.

@joshtrichards
Copy link
Member

I like the targeted approach! 👍

How does mod_php avoid this? Does it just buffer every transaction?

If I'm understanding this correctly:

  • this applies to fpm only
  • this could have implications for resource usage on proxies (but since this is narrowly targeted and these transactions failed before I guess it doesn't matter too much?)

Related issues:

If we proceed with this:

  • We should probably document it somewhere, particularly if there's a chance for resource usage trade-offs and/or if mod_php might be preferable in some environments over using this solution
  • Probably some work required for parity within the Nginx example config we provide in the Admin Manual since we explicitly disable the fastcgi_request_buffering behavior there
  • Might be some other touch spots in the docs - e.g. we reference the Sabre URL about 0-byte files currently in the Admin Manual (https://docs.nextcloud.com/server/latest/admin_manual/issues/general_troubleshooting.html#troubleshooting-webdav)

@joshtrichards joshtrichards added the pending documentation This pull request needs an associated documentation update label Jan 23, 2024
@artonge
Copy link
Contributor Author

artonge commented Jan 24, 2024

How does mod_php avoid this? Does it just buffer every transaction?

From what I understand, as it is closer to Apache, the communication is better, and mod_php is able to give the file content to Nextcloud. But I am not an expert at all, feel free to correct me :).

  • this applies to fpm only

And FastCGI, no ?

  • this could have implications for resource usage on proxies (but since this is narrowly targeted and these transactions failed before I guess it doesn't matter too much?)

It does, but as you said, this only targets specific requests, which are currently failing, so better than nothing I guess.

We should probably document it somewhere, particularly if there's a chance for resource usage trade-offs and/or if mod_php might be preferable in some environments over using this solution

Is it possible to add a condition to exclude mod_php ?

Probably some work required for parity within the Nginx example config we provide in the Admin Manual since we explicitly disable the fastcgi_request_buffering behavior there

Indeed, the issue also occurs with Nginx.

Might be some other touch spots in the docs - e.g. we reference the Sabre URL about 0-byte files currently in the Admin Manual (https://docs.nextcloud.com/server/latest/admin_manual/issues/general_troubleshooting.html#troubleshooting-webdav)

Better to keep it, no?

Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge force-pushed the artonge/feat/buffer_chunked_requests branch from be94125 to 3c0c373 Compare January 24, 2024 11:12
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

🐘 First time I hear about those things

@artonge artonge merged commit b71c037 into master Jan 25, 2024
44 checks passed
@artonge artonge deleted the artonge/feat/buffer_chunked_requests branch January 25, 2024 16:32
@nursoda
Copy link

nursoda commented Feb 18, 2024

The PR doesn't fix the issue for me on 28.0.3RC1. Do I also need to tweak my NGINX Nextcloud config to make it work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants