nginx: buffer responses in nginx, not application layer

[alxndrsn]

Closes #2029

What has been done to verify that this works as intended?

• added a new test
• deployed to a test server and demonstrated improved performance and cleaner failures for concurrent requests for large responses

Why is this the best possible solution? Were any other approaches considered?

• nginx disk RAM + space should be less expensive than postgres connection
• to avoid resource lock-up for responses over 100MB, they could by rejected by nginx

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

• This should be a big win for performance, but what happens when nginx runs out of disk space?

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

• branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
• verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

[lognaturel]

what happens when nginx runs out of disk space

My understanding is that if nginx runs out of disk space while trying to buffer a request to disk then that request aborts (ref).

We could consider setting proxy_max_temp_file_size low, maybe something like in the 10mb range. We expect most requests to be small. Once that limit is exceeded for a single request, backpressure will go back to the application as it is now. That limit is per-request so with many concurrent clients it would still be possible to exhaust temp space.

Or we keep proxy_max_temp_file_size to its default of 1Gb and accept that on servers with many concurrent clients and low disk space some connections will be aborted.

I don't have a big preference between the two above. Maybe a slight preference for the second because it sticks to defaults, and we can adjust as we learn more about how that behaves.

Pverall, his change looks like a good one to me. I really don't understand why the default was overridden in the first place. In the case of a client/connection that can keep up with the application server, nginx will just stream the response out, right? Maybe there's a small difference in time to first byte since it looks like nginx does fill an in-memory buffer before sending: https://mailman.nginx.org/pipermail/nginx/2011-October/029972.html Maybe proxy_buffering was interpreted as the whole response being buffered instead of just the first 4KB.

I do think it would be nice to do a sanity check on e.g. dev before merging. The nice thing would be to simulate a client with a slow connection because I imagine that's where we'll see the biggest difference. I imagine there will be very little change for fast clients.

[lognaturel]

to avoid resource lock-up for responses over 100MB, they could by rejected by nginx

Can you expand on this? Because many of the responses are streamed, I don't think any part of the system would know that a response would be over 100MB ahead of time.

[lognaturel]

@garethbowen when you're back could you please finish this one with @alxndrsn? No blockers from my end, just a couple of suggestions! Can't wait to see the impact it has.

[alxndrsn]

Can you expand on this? Because many of the responses are streamed, I don't think any part of the system would know that a response would be over 100MB ahead of time.

I think you're correct! I've crossed out that suggestion in the PR description.

[alxndrsn]

We could consider setting proxy_max_temp_file_size low, maybe something like in the 10mb range. We expect most requests to be small.

I'm happy with the 1GB default - I think big responses are the main cause of problems when proxy-buffering is disabled.

[garethbowen]

The nginx changes are great, more like best practice, and potentially great for performance.

I've got a couple of suggestions for the test that would give me more confidence that we don't regress later...

[garethbowen]

Comment inline. Mostly I just need to confirm my understanding of how the test works!

[garethbowen]

Beautiful!