Option to disable buffering in uwsgi module

[Peter Smit]

I am now getting around to create this patch. Would it be a good idea
to have the buffering option for uwsgi, scgi and fastcgi? Would you
like all three in different patches or in one?




2011/9/23 Maxim Dounin <mdounin at mdounin.ru>:
> Hello!
>
> On Fri, Sep 23, 2011 at 01:51:20PM +0300, Peter Smit wrote:
>
>> On Fri, Sep 23, 2011 at 1:31 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
>> >
>> > 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.
>> >
>>
>> End of next week I will look to this and maybe send in a new patch for
>> both uwsgi and scgi. Can I checkout somewhere the svn tip to make my
>> patch against? Or should I take the latest release code?
>
> You can checkout at svn://svn.nginx.org/nginx/trunk.  Though it's
> ok to use latest development release as well.
>
> Maxim Dounin
>
>
>>
>> > [...]
>> >
>> > > --- 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
>> >
>> > _______________________________________________
>> > nginx-devel mailing list
>> > nginx-devel at nginx.org
>> > http://mailman.nginx.org/mailman/listinfo/nginx-devel
>>
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel at nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel