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

HTTP/1.0 and keep-alive bug #1614

Closed
miquels opened this issue Jul 30, 2018 · 2 comments · Fixed by #1634
Closed

HTTP/1.0 and keep-alive bug #1614

miquels opened this issue Jul 30, 2018 · 2 comments · Fixed by #1634
Labels
A-http1 Area: HTTP/1 specific. C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor.

Comments

@miquels
Copy link

miquels commented Jul 30, 2018

I was testing my hyper-based server with ab (Apache Bench) and it turns out that if you enable the -k (keepalive) option, ab just hangs.

Turns out that ab uses HTTP/1.0 requests. It sends a Connection: keep-alive header, and hyper does keep the connection alive, but it does not add a Connection: keep-alive header to the reply. ab assumes that the server does not support keep-alive, and waits for the server to close the connection, and as a result ab hangs.

Hyper should probably add a Connection: keep-alive header to the response if it is found in the request, the request protocol is HTTP/1.0 and there's no Connection header in the Response object present yet.

@seanmonstar
Copy link
Member

Yikes, you're right! Seems there's two parts to this bug:

  • When the version is HTTP/1.0, and the response didn't send Connection: keep-alive, it should have closed the connection.
  • When coercing an HTTP/1.1 response to HTTP/1.0 because that's what the client speaks, a Connection: keep-alive header should be added.

Probably the best place to do this is in Conn::enforce_version. If enforcing 1.0, then check for the response version. If also 1.0, then check for Connection: keep-alive, and it not present, call self.state.disable_keep_alive(). If the response is 1.1, then translating to 1.1 should add Connection: keep-alive if self.state.wants_keep_alive() is true.

@seanmonstar seanmonstar added C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor. A-http1 Area: HTTP/1 specific. labels Jul 30, 2018
lambdasqd added a commit to lambdasqd/hyper that referenced this issue Aug 15, 2018
Change behaviour of connection or server response when the request is
version 1.0 and the Connection: keep-alive header is not present.

1. If the response is also version 1.0, then connection is closed if the
server keep-alive header is not present.
2. If the response is version 1.1, then the keep-alive header is added
when downgrading to version 1.0.

CLOSES: hyperium#1614
lambdasqd added a commit to lambdasqd/hyper that referenced this issue Aug 15, 2018
Change behaviour of connection or server response when the request is
version 1.0 and the Connection: keep-alive header is not present.

1. If the response is also version 1.0, then connection is closed if the
server keep-alive header is not present.
2. If the response is version 1.1, then the keep-alive header is added
when downgrading to version 1.0.

CLOSES: hyperium#1614
lambdasqd added a commit to lambdasqd/hyper that referenced this issue Aug 15, 2018
Change behaviour of connection or server response when the request is
version 1.0 and the Connection: keep-alive header is not present.

1. If the response is also version 1.0, then connection is closed if the
server keep-alive header is not present.
2. If the response is version 1.1, then the keep-alive header is added
when downgrading to version 1.0.

CLOSES: hyperium#1614
@lambdasqd
Copy link
Contributor

lambdasqd commented Aug 15, 2018

PR #1634 should fix this.

Here is a comparison of the Apache Bench output before and after.

before change

$ ab -k http://127.0.0.1:3000/
This is ApacheBench, Version 2.3 <$Revision: 1807734 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient)...apr_pollset_poll: The timeout specified has expired (70007)

after change

$ ab -k http://127.0.0.1:3000/
This is ApacheBench, Version 2.3 <$Revision: 1807734 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient).....done


Server Software:        
Server Hostname:        127.0.0.1
Server Port:            3000

Document Path:          /
Document Length:        12 bytes

Concurrency Level:      1
Time taken for tests:   0.001 seconds
Complete requests:      1
Failed requests:        0
Keep-Alive requests:    1
Total transferred:      112 bytes
HTML transferred:       12 bytes
Requests per second:    939.85 [#/sec] (mean)
Time per request:       1.064 [ms] (mean)
Time per request:       1.064 [ms] (mean, across all concurrent requests)
Transfer rate:          102.80 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     1    1   0.0      1       1
Waiting:        1    1   0.0      1       1
Total:          1    1   0.0      1       1

seanmonstar pushed a commit that referenced this issue Aug 15, 2018
Change behaviour of connection or server response when the request is
version 1.0 and the Connection: keep-alive header is not present.

1. If the response is also version 1.0, then connection is closed if the
server keep-alive header is not present.
2. If the response is version 1.1, then the keep-alive header is added
when downgrading to version 1.0.

Closes #1614
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http1 Area: HTTP/1 specific. C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants