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

Impossible to not Buffer Request Body with NGINX #2121

Open
SecurityInsanity opened this Issue Sep 19, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@SecurityInsanity

SecurityInsanity commented Sep 19, 2018

Issue report

Fill in as much as possible so that we can understand, find and fix the problem.

Are you sure this is a bug in Passenger? Yes. Passenger does not expose a way to configure this flag for the nginx module.

Question 1: What is the problem?

When attempting to process a request in a streaming fashion NGINX is currently buffering the entire request, before making a connection to the downstream app. This is controlled by the setting: request_body_no_buffering.

This seems to default to 1 HERE. However, if you notice there's this handy: if statement that seems to set it to 0, if there's any request body: HERE.

There's a couple NGINX modules that then set it back to 1 to support streaming. For example HERE in the proxy module.

However passenger doesn't seem to reference this setting anywhere, even when just grepping the repo:

$  rg 'request_body_no_buffering' .
$

This leads to the request always being buffered in nginx. Since this functionally defaults to 0 in nginx. This should happen with any architecture, but we've provided a reproduction with a simple nodejs app.

  • What is the expected behavior?

Passenger with nginx should expose a setting to allow nginx to not buffer requests before sending it on.

  • What is the actual behavior?

Passenger does not expose this setting.

  • How can we reproduce it? Please try to provide a sample application (or Virtual Machine) demonstrating the issue. Otherwise, if we can't reproduce it, we might have to ask you a number of followup questions or run certain commands to try and figure out the problem.

Be as detailed as possible in your descriptions, include any logs and stack traces (don't just cut/paste the error, provide some logging before that too).

(if you are requesting a feature instead of reporting an issue, describe here what you have in mind and how it would help you)

Your answer:

HERE is a gist for the nginx configuration/single node app file.
I've also provided a dockerfile in this app which installs everything to the app.

All I do is startup the docker file by building it, and then running it:

docker build . -t passenger-test && docker run --rm -p 8080:80 -i -t passenger-test

Then in another terminal I generate an empty large file (easy to see with files 15GB or more), and try to upload it:

dd if=/dev/zero of=output.dat  bs=1M  count=15000
curl -H "Transfer-Encoding: chunked" -d @output.dat -XPOST http://localhost:8080

You'll notice that even though curl is actively doing work. The server won't log the connection exists for quite awhile. This is because passenger is buffering the full file before passing it on to the node application.

Question 2: Passenger version and integration mode
Your answer:

  • Passenger Version: 5.3.3, Nginx v1.14.0

Question 3: OS or Linux distro, platform (including version):
Your answer:

  • Ubuntu 18.04 bionic beaver, NodeJS v10

Question 4: Passenger installation method:

Your answer:
[ ] RubyGems + Gemfile
[ ] RubyGems, no Gemfile
[X] Phusion APT repo
[ ] Phusion YUM repo
[ ] OS X Homebrew
[ ] source tarball
[ ] Other, please specify:

Question 5: Your app's programming language (including any version managers) and framework (including versions):
Your answer:

  • NodeJS v10, ExpressJS v4.16.3

Question 6: Are you using a PaaS and/or containerization? If so which one?
Your answer:

  • Docker

Question 7: Anything else about your setup that we should know?

Your answer: Not that I can think of.

@FooBarWidget

This comment has been minimized.

Member

FooBarWidget commented Oct 9, 2018

One use case for this is receiving large uploads as streams, where the app must begin a response before the request is finished so that the load balancer doesn't timeout the request.

@FooBarWidget FooBarWidget added this to the 5.3.6 milestone Oct 9, 2018

@FooBarWidget

This comment has been minimized.

Member

FooBarWidget commented Oct 10, 2018

I have scheduled this for the next version, 5.3.6.

@FooBarWidget

This comment has been minimized.

Member

FooBarWidget commented Nov 2, 2018

Work has begun on this issue.

Initial investigation reveals that implementing this feature requires not only setting the request_body_no_buffering flag (as the reporter mentions). It is also necessary to rechunk any body data that Nginx passes to the Passenger Core, otherwise it'll break Transfer-Encoding chunked requests. Implementing a data rechunker in the Nginx module is not easy and would take quite a bit of work.

FooBarWidget added a commit that referenced this issue Nov 3, 2018

Work in progress for issue #2121
We now always set request_body_no_buffering. We rely on the Core Controller's own buffering instead.
The biggest issue I had to deal with so far was writing an output filter. This output filter
is installed when dealing with chunked request bodies, and re-chunks data sent to the Controller
(because Nginx dechunks bodies).

The following integration tests still fail:
Phusion Passenger for Nginx a Ruby app running on the root URI it should behave like an example web app buffers uploads

[ci:skip]

@FooBarWidget FooBarWidget modified the milestones: 5.3.6, 6.0.0 Nov 5, 2018

@FooBarWidget

This comment has been minimized.

Member

FooBarWidget commented Nov 21, 2018

The investigation today revealed that part of the reason why my change from last time did not work, is due to a bug in Nginx itself. Specifically, nginx/nginx@62821f1, which is part of version 1.15.3. The bug manifests itself when more the request body is received over more than one request buffer. The bug in Nginx causes Nginx to enter an infinite loop due to the fact that it incorrectly iterates over a linked list.

This means that if we implement this feature request, then we need to bump our minimum supported Nginx version to 1.15.3.

@FooBarWidget

This comment has been minimized.

Member

FooBarWidget commented Nov 21, 2018

One complication is that Ubuntu 18.04 does not supply such a recent Nginx version. So implementing this feature request also presents packaging challenges.

FooBarWidget added a commit that referenced this issue Nov 26, 2018

FooBarWidget added a commit that referenced this issue Nov 26, 2018

FooBarWidget added a commit that referenced this issue Nov 27, 2018

FooBarWidget added a commit that referenced this issue Nov 27, 2018

Mention GH-2121 in CHANGELOG
[ci:skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment