Forum: NGINX websockets support for uwsgi protocol

Hi, the (tiny) attached patch enable support for new websockets handling
when the uwsgi protocol is used instead of HTTP.

I have tested it with various websocket libraries and with the api
available in uWSGI 1.9.

From 1.9 sources (with nginx pointing to uwsgi port 3031):

./uwsgi -s :3031 -w tests.websockets_echo --gevent 10


no additional configuration is needed for nginx

Hello!

(Cc'd nginx-devel@ as this is better list for this discussion.)

On Wed, Feb 20, 2013 at 02:30:38PM +0100, Roberto De Ioris wrote:

>
> no additional configuration is needed for nginx

Should we also do the same for SCGI?

I personally think it is more or less the same for all CGI-based
protocols, and the only problematic one is FastCGI, which probably
still needs wrapping within FCGI_STDIN/FCGI_STDOUT records instead
of real connection upgrade.

--
Maxim Dounin
http://nginx.com/support.html

>> available in uWSGI 1.9.
> I personally think it is more or less the same for all CGI-based
> protocols, and the only problematic one is FastCGI, which probably
> still needs wrapping within FCGI_STDIN/FCGI_STDOUT records instead
> of real connection upgrade.
>
>

AFAIK scgi body management works in the same way as the uwsgi one, so 
the
patch should be usable there too


--
Roberto De Ioris
http://unbit.it

>>>
>> Should we also do the same for SCGI?
>>
>> I personally think it is more or less the same for all CGI-based
>> protocols, and the only problematic one is FastCGI, which probably
>> still needs wrapping within FCGI_STDIN/FCGI_STDOUT records instead
>> of real connection upgrade.
>>
>>
>
> AFAIK scgi body management works in the same way as the uwsgi one, so the
> patch should be usable there too


Ok, i can confirm the same patch applied to the SCGI module in the same
position works (at least with uWSGI 1.9 in SCGI mode)

./uwsgi --scgi-nph-socket :3031 -w tests.websockets_echo --gevent 10

remember to add

scgi_param PATH_INFO $document_uri;

to the nginx config to make it work


--
Roberto De Ioris
http://unbit.it

Hello!

On Wed, Feb 20, 2013 at 03:07:53PM +0100, Roberto De Ioris wrote:

> >>> when the uwsgi protocol is used instead of HTTP.
> >>
> > patch should be usable there too
>
> to the nginx config to make it work

Ok, so the next question is: any specific reason to exclude normal
CGI responses with "Status" as in your patch?

I in fact don't like the idea of supporting http-like answers with
status like from CGI-like protocols, correct way is to use
"Status" header.  Not sure why Manlio introduced it at all,
probably due to some compatibility concerns (and due to the fact
that SCGI specification explicitly refuses to specify response
format).

Something like this should be better, IMHO:

diff --git a/src/http/modules/ngx_http_scgi_module.c 
b/src/http/modules/ngx_http_scgi_module.c
--- a/src/http/modules/ngx_http_scgi_module.c
+++ b/src/http/modules/ngx_http_scgi_module.c
@@ -984,7 +984,7 @@ ngx_http_scgi_process_header(ngx_http_re
             u = r->upstream;

             if (u->headers_in.status_n) {
-                return NGX_OK;
+                goto done;
             }

             if (u->headers_in.status) {
@@ -1015,6 +1015,14 @@ ngx_http_scgi_process_header(ngx_http_re
                 u->state->status = u->headers_in.status_n;
             }

+        done:
+
+            if (u->headers_in.status_n == NGX_HTTP_SWITCHING_PROTOCOLS
+                && r->headers_in.upgrade)
+            {
+                u->upgrade = 1;
+            }
+
             return NGX_OK;
         }

diff --git a/src/http/modules/ngx_http_uwsgi_module.c 
b/src/http/modules/ngx_http_uwsgi_module.c
--- a/src/http/modules/ngx_http_uwsgi_module.c
+++ b/src/http/modules/ngx_http_uwsgi_module.c
@@ -1018,7 +1018,7 @@ ngx_http_uwsgi_process_header(ngx_http_r
             u = r->upstream;

             if (u->headers_in.status_n) {
-                return NGX_OK;
+                goto done;
             }

             if (u->headers_in.status) {
@@ -1049,6 +1049,14 @@ ngx_http_uwsgi_process_header(ngx_http_r
                 u->state->status = u->headers_in.status_n;
             }

+        done:
+
+            if (u->headers_in.status_n == NGX_HTTP_SWITCHING_PROTOCOLS
+                && r->headers_in.upgrade)
+            {
+                u->upgrade = 1;
+            }
+
             return NGX_OK;
         }


--
Maxim Dounin
http://nginx.com/support.html

>
> Ok, so the next question is: any specific reason to exclude normal
> CGI responses with "Status" as in your patch?
>
> I in fact don't like the idea of supporting http-like answers with
> status like from CGI-like protocols, correct way is to use
> "Status" header.  Not sure why Manlio introduced it at all,
> probably due to some compatibility concerns (and due to the fact
> that SCGI specification explicitly refuses to specify response
> format).

Honestly i do not remember why Manlio added support for nph (but i have
added it to uWSGI SCGI parser too, so in my subconsciuous there should 
be
a good reason :P)

regarding your updated patch is better for sure

--
Roberto De Ioris
http://unbit.it

Hello!

On Wed, Feb 20, 2013 at 04:20:29PM +0100, Roberto De Ioris wrote:

> > format).
>
> Honestly i do not remember why Manlio added support for nph (but i have
> added it to uWSGI SCGI parser too, so in my subconsciuous there should be
> a good reason :P)
>
> regarding your updated patch is better for sure

Committed, thnx.

--
Maxim Dounin
http://nginx.com/support.html