Websockets support for uwsgi protocol

Roberto_De_Ioris · February 20, 2013, 2:31pm

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

Roberto_De_Ioris · February 20, 2013, 2:51pm

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 D.

Roberto_De_Ioris · February 20, 2013, 2:53pm

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

Roberto_De_Ioris · February 20, 2013, 3:08pm

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

Roberto_De_Ioris · February 20, 2013, 4:12pm

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 D.

Roberto_De_Ioris · February 20, 2013, 5:42pm

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 D.

Roberto_De_Ioris · February 20, 2013, 4:21pm

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