87047 - Don't expect connetion: headers from http/1.1 servers; and send proxy-connection, not connection, to proxy servers

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[phil.anderton]

According to rfc2616, persistent connections are the default for HTTP/1.1, but 
Mozilla sends a "Connection: close" header unless the 
preference "network.http.keep-alive" is set.

Reproducible: Always
Steps to Reproduce:
1. In Preferences/Debug/Networking:
2. Deselect "Enable Keep-Alive"
3. Set "HTTP Version" to "1.1".
4. Make any HTTP GET request and examine headers.

Expected Results:  When Mozilla is sending an HTTP/1.1 request it should never 
send "Connection: close". The "Connection: keep-alive" and "Keep-alive" headers 
are unnecessary and undesirable in HTTP 1.1 - see rfc2616 
19.6.2, "Compatibility with HTTP/1.0 Persistent Connections".

Comment 1

[Bradley Baetz (:bbaetz)]

The keep-alive pref is a debug-only pref designed for diagnosing and isolating
problems with persistent http transactions. When the pref is off (and it is on
by default), we don't want persistent connections at all, which is why we send
Connection: close. The pref could possibly be renamed to
network.http.persistent-connection but since its only purpose is for debugging
there's not much point.

We send Connection: Keep-Alive, and the Keep-Alive: header for compatablility
with older servers. You do have a point that 19.6.2 points out what is wrong
with doing so (although it does not actually forbid it, or in fact mention
http/1.1 clients at all). However, since ns4 and ie also send these headers, I'm
not sure that any proxy server will break in this way, especially when combined
with the rules for http/1.1 servers in 14.10.

Are you aware of anything which breaks in this case?

->darin

Comment 2

[phil.anderton]

I agree that there is no point renaming a debugging pref - but what about other 
prefs such as network.http.keep-alive.max-connections? (I'll be commenting on 
bug 83526 soon)

I also understand and agree with your decision to send the keep-alive headers 
for compatibility with older servers. The case I would like to see fixed is 
when using HTTP/1.1 together with a proxy. If the proxy is 1.1-compliant, the 
headers are superfluous; if not, the problem described in 19.6.2 could occur. 
Sorry, I don't have an actual example.

As a point of information, MSIE 5.5 doesn't send "Connection: Keep-Alive" 
and "Keep-Alive" when using HTTP/1.1 together with a proxy - it sends "Proxy-
Connection: Keep-Alive", which I understand was originally a 
Netscape "extension" to HTTP :-)

Comment 3

[Bradley Baetz (:bbaetz)]

Well, we probably should fix it for proxies, although, as I mentioned to darin
last night, that won't help us for tranparent proxies.

Changing aummary, and confirming.

Comment 4

[Darin Fisher]

Attachment: FWIW a patch which makes us only send a Connection header in the "close" case.

Comment 5

[Darin Fisher]

with this patch we obviously fail to ever open keep-alive connections to 
HTTP/1.0 servers.  i'm only submitting it for the sake of testing, etc.

Comment 6

[basic]

does bug 84264 really depend on this?

Comment 7

[Doug Turner (:dougt)]

bbaetz, can you review darin's patch?  

Comment 8

[Bradley Baetz (:bbaetz)]

dougt: Its not complete - see darin's comments. I could make it conditional on
us talking to a proxy server - would that work? We then wouldn't do keep-alive
to a 1.0 proxy server that doesn't understand 1.1 (ie squid works)

I'll check what IE does when neeti's machine finishes installing SP2, and I can
run the packet sniffer.

Comment 9

[Bradley Baetz (:bbaetz)]

OK, so IE uses proxy-connection: keep-alive when it talks to a proxy, and honors
proxy-connection: close.

That header is non-standard, and I can't find any docs on it, except a few
mailing list archives which point out its flaws (basically the same as are in
the bottom of the HTTP/1.1 RFC, applied to multi-hop proxies)
So my choice would be to send proxy-connection instead of connection, when
talking to a proxy. This will mean that (compared to the current situation) we
won't have keep-alives if we talk to an HTTP/1.0 proxy which doesn't understand
HTTP/1.1 and which understands connection: keep-alive, but not the
proxy-connection: keep-alive. I don't know of any servers which fall into that
category, however.

Does that seem reasonable?

Comment 10

[phil.anderton]

Seems very reasonable to me - servers which fall into the category you describe 
(if they exist) will have the same problem with IE.

Comment 11

[Bradley Baetz (:bbaetz)]

So, it turns out that Darin's patch does more that I thought it did. I've just
changed it to send Connection/Proxy-Connection headers as appropriate.

And, we now use keep-alives to lots of sites. Like those using netscape
enterprise server. And those using IIS. And www.google.com. (apache results
don't change)

Adding perf - this cuts down the number of packets involved in downloading
www.cnn.com from 900 to about 610. Using john taylor's page loader test, we
appear to improve in speed between 5% to 30% on www.cnn.com (numbers are very
variable, and were generated on a -O2 --enable-debug build)

Someone want to try some ibench numbers with and without this patch?

gagan/dougt - r/sr?

Comment 12

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 13

[Darin Fisher]

bbaetz: what if the proxy type is ssl?

Comment 14

[Darin Fisher]

you should check if the proxyType is "http" (i think)

Comment 15

[Bradley Baetz (:bbaetz)]

Attachment: and after yesterday I tried not to break socks....

Comment 16

[benc]

Proxy-connectinon: I think they are mentioned in Ari's Web Proxy book, (which I
lost my copy of...) tever has a copy on his desk :)

More importantly, what proxy servers are following this line?

Comment 17

[Bradley Baetz (:bbaetz)]

OK, ibench numbers didn't change over the LAN.

benc: No, they're not in that book, but squid accepts them, and does the right
thing. In the future, we may consider just not sending these headers, but we're
not going to make that change now.

Comment 18

[Gagan]

r=gagan on the last patch.
Please verify this with a dialup connection to see if this makes a bigger splash 
on ibench numbers and that will give us an idea if this is a candidate for the 
branch.

Comment 19

[Bradley Baetz (:bbaetz)]

-> dougt for sr and checkin to the trunk early next week

jrgm, this is the bug we talked about

Comment 20

[Bradley Baetz (:bbaetz)]

retaking/targeting. I'll nag dougt again for an sr this week....

Comment 21

[Jens-Uwe]

might this causes things like bug 89875?

Comment 22

[Bradley Baetz (:bbaetz)]

possibly, although I'm not certain. Its probably related though.

Comment 23

[Bradley Baetz (:bbaetz)]

Just under 5% better performance on ibench over a 50K modem connection.

Comment 24

[Bradley Baetz (:bbaetz)]

checked in on trunk.

Comment 25

[Bradley Baetz (:bbaetz)]

... and marking fixed

Comment 26

[Bradley Baetz (:bbaetz)]

embedding wants this for their release. nominating nsBranch

Comment 27

[Bradley Baetz (:bbaetz)]

[e-mail] pointed out in bug 84264 that our default timeout should be
the same as what we send in the keep-alive header, ie 300 seconds. This seems
long, but we already detect when the server closes teh connection before we
write to it, and we really want 'infinite' for http/1.1, so this isn't a
problem. Patch coming up - see bug 91024.

This patch needs to go on the branch in addition to the other one.

Comment 28

[Bradley Baetz (:bbaetz)]

Attachment: part 2

Comment 29

[Gagan]

r=gagan on the last patch.

Comment 30

[Patrick McManus [:mcmanus]]

a timeout of 300 is way too big.. practically speaking 
you'll force servers to do the active
close.. remember that the side who does the active close needs to go into 
TIME_WAIT.. and given that there are >=1 clients and exactly 1 server, its
much nicer to have clients doing active closes.

fwiw, the raw # isn't particularly interesting on the server side.. I've never
seen a need to consult it in my applications... there really is no reason
to send one, the more interesting thing is to decide when to give up on an
idle socket.

msie doesn't send a specifc parameter, but its timeouts seem tied to 
click patterns (i.e. closes a lot of open sessions when a page is fully
rendered) and this works well.

Comment 31

[Bob McElrath]

I don't really care about the value of the timeout, but I want it to be possible
for a proxy to keep a single connection open for an infinite amount of time, as
long as Mozilla keeps requesting pages.  Mozilla should obey 'Connection:
keep-alive' and 'Keep-Alive: <seconds>' when sent with a response from a proxy
(or server), and Mozilla should not ever close a connection for any (non-error)
reason except timeout (see bug 84264).  Of course, the server may close the
connection.  10s timeout to server sounds reasonable to me.  

Really, the default timeout should be set to ~twice the average amount of time
it takes Mozilla to parse your average page, figure out which images are in that
page, and make requests for those images, on a reasonably slow line.

Comment 32

[Bradley Baetz (:bbaetz)]

Please make these comments in bug 91024. After this patch, the timeout will be
300 seconds (ie 5 minutes) idle by default. Its already a pref.

We know that the behaviour is not pefect, but its now better than not reusing
the connections at all. The server can still close it earlier, and we will
respect that. We may need more than just 'keep it open for n seconds', but this
is not the bug to talk about that.

Comment 33

[Bradley Baetz (:bbaetz)]

dougt says sr=dougt.

Oh, and I did mean bug 91024. Oops.

Comment 34

[Bradley Baetz (:bbaetz)]

3rd try lucky - bug 91204 is what I meant. No, really :)

Comment 35

[Bradley Baetz (:bbaetz)]

pushing out milestone to avoid getting spammed - this is checked in on the trunk
already, and so the milestone doesn't really make sense.

Comment 36

[Bradley Baetz (:bbaetz)]

checked in on 092 branch.