Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send HTTP-1.1 Responses. #31

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ssmccoy
Copy link

@ssmccoy ssmccoy commented Aug 13, 2012

  • Despite not requiring the Host header or supporting keep-alives, send
    HTTP/1.1 response.
    • This will allow custom servers to actually use some HTTP/1.1 features,
      such as the cache-control header, even though clients are forced to
      delegate keep-alive behavior to their load-balancer.
    • When tested behind haproxy and friends, has no impact on upstream
      load-balancing.

Scott S. McCoy added 2 commits August 13, 2012 14:23
 * Despite not requiring the ``Host`` header or supporting keep-alives, send an
   HTTP/1.1 response.
   * This will allow custom servers to actually use some HTTP/1.1 features,
     such as the cache-control header, even though clients are forced to
     delagate keep-alive behavior to their load-balancer.
   * When tested behind haproxy and friends, has no impact on upstream
     load-balancing.
@miyagawa
Copy link
Owner

I haven't fully grokked the patch yet, but i think it should respond 1.1 only if the request in question is in 1.1.

@ssmccoy
Copy link
Author

ssmccoy commented Aug 27, 2012

That's certainly not required. Here's what apache does:

GET / HTTP/1.0

HTTP/1.1 200 OK
Date: Mon, 27 Aug 2012 02:50:42 GMT
Server: Apache/2.2.14 (Ubuntu) DAV/2 SVN/1.6.6 mod_python/3.3.1 Python/2.6.5

@vti
Copy link

vti commented Sep 3, 2012

+1

With 1.0 no chunked and friends :(

@vti
Copy link

vti commented Sep 3, 2012

Btw, why add Connection close? Isn't that only for 1.0?

@ssmccoy
Copy link
Author

ssmccoy commented Sep 3, 2012

You have to send Connection: close if you are going to close the connection. Otherwise HTTP/1.1 is keep-alive by default and you may wind up with some pipelined requests that will get dropped when you close the connection.

I didn't want to go through and implement keep-alives, because it was too much effort at this layer. In my current deployment, I push several Twiggy daemon processes to each box and have a local load balancer on the instance. The load balancer does keep-alives for me upstream.

Adding real keep alive support would be a significant change.

@vti
Copy link

vti commented Sep 3, 2012

I am implementing sockjs port and need to use chunked encoding almost
everywhere, but when using Plack middleware to do so it looks for
connection header presence. So this 'close' messes up things. Maybe that's
middleware problem and it should look for a http version... Having
keep-alive in Twiggy would be nice though.
03.09.2012 23:45 ÐÏÌØÚÏ×ÁÔÅÌØ "Scott S. McCoy" [email protected]
ÎÁÐÉÓÁÌ:

You have to send Connection: close if you are going to close the
connection. Otherwise HTTP/1.1 is keep-alive by default and you may wind up
with some pipelined requests that will get dropped when you close the
connection.

I didn't want to go through and implement keep-alives, because it was too
much effort at this layer. In my current deployment, I push several Twiggy
daemon processes to each box and have a local load balancer on the
instance. The load balancer does keep-alives for me upstream.

Adding real keep alive support would be a significant change.

Reply to this email directly or view it on GitHubhttps://github.com//pull/31#issuecomment-8246714.

@vti
Copy link

vti commented Sep 3, 2012

Actually it doesn't look for connection, my mistake, but it doesn't remove
it either.
04.09.2012 0:39 ÐÏÌØÚÏ×ÁÔÅÌØ "vti" [email protected] ÎÁÐÉÓÁÌ:

I am implementing sockjs port and need to use chunked encoding almost
everywhere, but when using Plack middleware to do so it looks for
connection header presence. So this 'close' messes up things. Maybe that's
middleware problem and it should look for a http version... Having
keep-alive in Twiggy would be nice though.
03.09.2012 23:45 ÐÏÌØÚÏ×ÁÔÅÌØ "Scott S. McCoy" [email protected]
ÎÁÐÉÓÁÌ:

You have to send Connection: close if you are going to close the
connection. Otherwise HTTP/1.1 is keep-alive by default and you may wind up
with some pipelined requests that will get dropped when you close the
connection.

I didn't want to go through and implement keep-alives, because it was too
much effort at this layer. In my current deployment, I push several Twiggy
daemon processes to each box and have a local load balancer on the
instance. The load balancer does keep-alives for me upstream.

Adding real keep alive support would be a significant change.

Reply to this email directly or view it on GitHubhttps://github.com//pull/31#issuecomment-8246714.

@ssmccoy
Copy link
Author

ssmccoy commented Sep 4, 2012

In order to support HTTP/1.1 correctly, we will need to send the
Connection: close header. Otherwise the connection is implicitly
keep-alive. The only options are stick with HTTP/1.0 (ignoring the
keep-alive extensions), implement keep-alive, or send "Connection: close"
each time.

On Mon, Sep 3, 2012 at 2:55 PM, Viacheslav Tykhanovskyi <
[email protected]> wrote:

Actually it doesn't look for connection, my mistake, but it doesn't remove
it either.
04.09.2012 0:39 ÐÏÌØÚÏ×ÁÔÅÌØ "vti" [email protected] ÎÁÐÉÓÁÌ:

I am implementing sockjs port and need to use chunked encoding almost
everywhere, but when using Plack middleware to do so it looks for
connection header presence. So this 'close' messes up things. Maybe
that's
middleware problem and it should look for a http version... Having
keep-alive in Twiggy would be nice though.
03.09.2012 23:45 ÐÏÌØÚÏ×ÁÔÅÌØ "Scott S. McCoy" [email protected]

ÎÁÐÉÓÁÌ:

You have to send Connection: close if you are going to close the
connection. Otherwise HTTP/1.1 is keep-alive by default and you may
wind up
with some pipelined requests that will get dropped when you close the
connection.

I didn't want to go through and implement keep-alives, because it was
too
much effort at this layer. In my current deployment, I push several
Twiggy
daemon processes to each box and have a local load balancer on the
instance. The load balancer does keep-alives for me upstream.

Adding real keep alive support would be a significant change.

Reply to this email directly or view it on GitHub<
https://github.com/miyagawa/Twiggy/pull/31#issuecomment-8246714>.


Reply to this email directly or view it on GitHubhttps://github.com//pull/31#issuecomment-8247819.

Cheers,
Scott S. McCoy

@dolmen
Copy link

dolmen commented Nov 25, 2014

@vti What is the status of your fork for SockJS?

@vti
Copy link

vti commented Nov 25, 2014

@dolmen usable under patched Feersum :) But I am doing some internal refactoring right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants