Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

HTTP 1.0 questions (derived from SockJS) #302

Closed
majek opened this Issue Oct 30, 2012 · 6 comments

Comments

Projects
None yet
2 participants

majek commented Oct 30, 2012

@essen, during this bug sockjs/sockjs-protocol#57 we found that either SockJS or Cowboy behave weirdly for Http 1.0. Here's what I found:

  1. New lines dos vs unix style.

Correct me if I'm wrong, but this looks like a valid http 1.0 request, right?

$  echo -en "GET /echo HTTP/1.0\n\n" | nc localhost 8081 -q 2
HTTP/1.1 505 HTTP Version Not Supported

(hint: works fine with \r\n instead of \n)

  1. Cowboy doesn't respond with 'keep-alive' although it does agree to it:
$ echo -en "GET /echo HTTP/1.0\r\nConnection: Keep-Alive\r\n\r\n" | nc localhost 8081 -q 2
HTTP/1.0 200 OK
server: Cowboy
date: Tue, 30 Oct 2012 20:02:45 GMT
content-length: 19
Content-Type: text/plain; charset=UTF-8

Welcome to SockJS!

This hangs server for two seconds - all good. The repsponse is valid, and the server waits for next request (ie: it understood keep-alive). But it doesn't specify 'connection: keep-alive' in the response. It should OR it should close the connection.

I suggest to include "connection" header always.

Owner

essen commented Oct 30, 2012

\n isn't valid, no. The spec specifies \r\n only.

It should close the connection. I will fix that. Thanks!

majek commented Oct 30, 2012

You got me! Http 1.0 rfc indeed says: a bare CR or LF should not be substituted for CRLF

Owner

essen commented Nov 27, 2012

Does the following patch fix your issue?

diff --git a/src/cowboy_req.erl b/src/cowboy_req.erl
index 3426dbc..2d45a59 100644
--- a/src/cowboy_req.erl
+++ b/src/cowboy_req.erl
@@ -183,15 +183,13 @@ new(Socket, Transport, Method, Path, Query, Fragment,
        method=Method, path=Path, qs=Query, fragment=Fragment, version=Version,
        headers=Headers, host=Host, port=Port, buffer=Buffer,
        onresponse=OnResponse},
-   case CanKeepalive of
+   case CanKeepalive and (Version =:= {1, 1}) of
        false ->
            Req#http_req{connection=close};
        true ->
            case lists:keyfind(<<"connection">>, 1, Headers) of
-               false when Version =:= {1, 1} ->
-                   Req; %% keepalive
                false ->
-                   Req#http_req{connection=close};
+                   Req; %% keepalive
                {_, ConnectionHeader} ->
                    Tokens = parse_connection_before(ConnectionHeader, []),
                    Connection = connection_to_atom(Tokens),

majek commented Nov 27, 2012

Yup, seems to be working fine.

Owner

essen commented Nov 27, 2012

Cool, I'll merge that then, thanks!

Owner

essen commented Nov 27, 2012

Done in 5bc5f56. Thanks!

@essen essen closed this Nov 27, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment