Option to disable buffering in uwsgi module

[Maxim Dounin]

Hello!

On Fri, Sep 23, 2011 at 09:24:29AM +0300, Peter Smit wrote:

> >
> > On Thu, Aug 04, 2011 at 01:32:14PM +0300, Peter Smit wrote:
> > > (This is my first message to this list, please let me know if I'm
> > > doing something wrong!)
> > >
> > > In developing a comet style application in a uwsgi/nginx setup I
> > > noticed that nginx always buffers the response when uwsgi_pass is
> > > used.
> > >
> > > I'm not sure or there is any particular reason why this is done,
> > > except for the fact that the uwsgi code is originally based on the
> > > fastcgi module where indeed buffering is unavoidable. I think however
> > > that it makes sense to give the option of disabling buffering with
> > > uwsgi.
> > >
> > > I actually already went ahead and wrote a patch that does exactly
> > > this. It introduces a uwsgi_buffering flag and now adheres to the
> > > "X-Accel-Buffering" header. I have only limited capabilities to test
> > > this patch, but for me it does exactly that, disabling the buffer.
> > >
> > > Could some of you review this patch and if it is ok, could it be
> > > introduced in nginx?
> > >
> > > I made the patch on the 1.1.0 source. I attached it and included it
> > > inline below this message. Let me know if I should give it in a
> > > different format.
> >
> > Thank you for idea, it seems unbuffered proxying should work for uwsgi
> > as well as for scgi, since they both use simple protocol, that is, no
> > protocol at all :) for body.
> >
> >
> >
> How does the process go further? I noticed that the patch is not included
> yet. Are you waiting for someone to step up to make a patch also for scgi,
> or are there other reasons for not applying the patch yet?

The patch wasn't yet considered due to a number of upstream 
infrasture changes related to upstream keepalive support.  Now 
with upstream keepalive committed we may proceed with the patch.

Could you please provide patch for scgi as well?

And please see review below, there are some minor nits which 
should be addressed.

[...]

> --- nginx-1.1.0/src/http/modules/ngx_http_uwsgi_module.c        2011-07-29
> 18:33:03.000000000 +0300
> +++ nginx-1.1.0-uwsgi_buffering/src/http/modules/ngx_http_uwsgi_module.c
>    2011-08-04
> 13:16:54.381528459 +0300
> @@ -123,6 +123,13 @@
>       offsetof(ngx_http_uwsgi_loc_conf_t, upstream.store_access),
>       NULL },
> 
> +    { ngx_string("uwsgi_buffering"),
> +
>  NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE123,

This shouldn't be TAKE123, but NGX_CONF_FLAG instead.

> +      ngx_conf_set_flag_slot,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      offsetof(ngx_http_uwsgi_loc_conf_t, upstream.buffering),
> +      NULL },
> +
>     { ngx_string("uwsgi_ignore_client_abort"),
>       NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_FLAG,
>       ngx_conf_set_flag_slot,
> @@ -445,7 +452,7 @@
>     u->abort_request = ngx_http_uwsgi_abort_request;
>     u->finalize_request = ngx_http_uwsgi_finalize_request;
> 
> -    u->buffering = 1;
> +    u->buffering = uwcf->upstream.buffering;

It looks like posted patch suffers from whitespace corruption, 
likely by mail client.  You may want to attach it instead.

> 
>     u->pipe = ngx_pcalloc(r->pool, sizeof(ngx_event_pipe_t));
>     if (u->pipe == NULL) {
> @@ -1083,6 +1090,8 @@
>     /* "uwsgi_cyclic_temp_file" is disabled */
>     conf->upstream.cyclic_temp_file = 0;
> 
> +    conf->upstream.change_buffering = 1;
> +
>     ngx_str_set(&conf->upstream.module, "uwsgi");
> 
>     return conf;

(just for record to myself: configuration merge for 
upstream.buffering is already present in both uwsgi and scgi 
modules, not needed in the patch)

Maxim Dounin