Skip to content

Conversation

@caburj
Copy link

@caburj caburj commented Oct 24, 2025

Because of multi-layered use of nginx, responses with "X-Accel-Buffering: no" is still buffered because the special header is absorbed by layer just above the odoo app. As a result, it's impossible to test in runbot endpoints that stream responses. To ensure that header is respected thoughout the stack, we propose in this commit to pass the "X-Accel-Buffering" header.

Because of multi-layered use of nginx, responses with "X-Accel-Buffering: no"
is still buffered because the special header is absorbed by layer just above the
odoo app. As a result, it's impossible to test in runbot endpoints that stream
responses. To ensure that header is respected thoughout the stack, we propose in
this commit to pass the "X-Accel-Buffering" header.
@C3POdoo C3POdoo requested a review from a team October 24, 2025 12:47
Comment on lines +94 to +97
location / {
proxy_pass_header "X-Accel-Buffering";
proxy_pass http://127.0.0.1:<t t-out="build.port"/>;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a very bad idea.

First the code is not correct, proxy_pass_header is about adding a header on on request to be forwarded, not about adding a header on the response. You meant add_header.

But preventing nginx from buffering most responses (location /) is absolutely madness. Don't do that.

Copy link
Member

Choose a reason for hiding this comment

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

There should be some code that check for the presence of the X-Accel-Buffering header on the original response, and repeat it on the forwarded response.

Copy link
Author

Choose a reason for hiding this comment

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

My goal is to stream response and prevent the buffering so that the client won't receive the streamed data at once, thus, I'm adding an nginx custom header "x-accel-buffering: no" which is actually suggested from the docs of proxy_buffering (https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffering).

"Buffering can also be enabled or disabled by passing “yes” or “no” in the “X-Accel-Buffering” response header field."

First the code is not correct, proxy_pass_header is about adding a header on on request to be forwarded, not about adding a header on the response. You meant add_header.

The docs (https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_pass_header) says otherwise:

proxy_pass_header: "Permits passing otherwise disabled header fields from a proxied server to a client."

Note: server to client so it pertains to the response. There is proxy_pass_request_header for requests (https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_pass_request_headers).

But preventing nginx from buffering most responses (location /) is absolutely madness. Don't do that.

proxy_buffering is "on" by default. To escape the buffering for specific responses, they suggest to add a response header called "x-accel-buffering: no". This is what I'm doing in the referenced enterprise PR. I'm just exceptionally disabling buffering for a certain endpoint. I'm not disabling the buffering for location /. I didn't put proxy_buffering off;.

Copy link
Member

Choose a reason for hiding this comment

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

You are right about everything! I got proxy_pass_header mixed with proxy_set_header. What does it mean for X-Accel-Redirect? that specific header should be interpreted by nginx and not copied to the next hop.

Copy link
Author

@caburj caburj Oct 30, 2025

Choose a reason for hiding this comment

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

I can't imagine a connection between "X-Accel-Buffering" and "X-Accel-Redirect", they seem independent. But I suppose "X-Accel-Redirect" isn't affected by the proposed change because the change is very specific to "X-Accel-Buffering".

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.

3 participants