Last Comment Bug 643352 - Keep-Alive header syntax invalid
: Keep-Alive header syntax invalid
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla5
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on:
Blocks: http-fingerprint
  Show dependency treegraph
 
Reported: 2011-03-20 19:14 PDT by mnot
Modified: 2011-05-08 03:15 PDT (History)
11 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
no-keep-alive-request-header v1 (1.61 KB, patch)
2011-03-21 10:50 PDT, Patrick McManus [:mcmanus]
bzbarsky: review+
Details | Diff | Splinter Review

Description mnot 2011-03-20 19:14:28 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-au) AppleWebKit/533.20.25 (KHTML, like Gecko) Version/5.0.4 Safari/533.20.27
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-GB; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13

FireFox sends a Keep-Alive request header in the following form:
  Keep-Alive: 115

However, Keep-Alive is defined by RFC2068 here:
  http://tools.ietf.org/html/rfc2068#section-19.7.1.1
as having the following syntax:

  Keep-Alive-header = "Keep-Alive" ":" 0# keepalive-param
  keepalive-param = param-name "=" value

I.e., the syntax is invalid.

Apache currently generates the correct syntax (e.g., Keep-Alive: timeout=120).

While it isn't the end of the world that this is invalid (because AFAICT this header isn't consumed by much software), the variance between Apache and FireFox is being used as an argument for minting Yet Another Header, which is unfortunate (esp. since it would waste Yet More Bytes). See:
  http://www.w3.org/mid/8B0A9FCBB9832F43971E38010638454F0400BEFED4@SISPE7MB1.commscope.com

Note also that the IETF HTTPbis WG has an open issue about re-defining Keep-Alive:
  http://trac.tools.ietf.org/wg/httpbis/trac/ticket/158
If Mozilla has input, it would (as always) be welcome.

Reproducible: Always

Steps to Reproduce:
1. Send a persistent request
2. Note the request headers (e.g., with tcpflow)
3. Observe a Keep-Alive header
Actual Results:  
Invalid syntax

Expected Results:  
Valid syntax
Comment 1 Boris Zbarsky [:bz] 2011-03-20 20:14:45 PDT
Awesome.  A bug dating back to 2001...

This is pretty trivial to change as desired; see http://hg.mozilla.org/mozilla-central/file/8618a149ea2e/netwerk/protocol/http/nsHttpHandler.cpp#l404

Patrick, Jason, what's the behavior we actually want here?
Comment 2 Patrick McManus [:mcmanus] 2011-03-21 06:06:53 PDT
(In reply to comment #1)

> Patrick, Jason, what's the behavior we actually want here?


If we're going to make a change, we should probably just go with the straight Connection: keep-alive version without a separate keep-alive request header. That would match chrome and ie too.

We don't do a very good job of honoring our 115 second number anyhow.. anytime we have hit out default global 30 connection limit and then need another one (which happens a lot!) we start closing idle persistent connections before that timer is expired.

Of course, we ought to raise that number from 30 because idle connections have value at very little cost to the client, but that's a different bug.

Mark, fwiw, we do parse the response Keep-Alive header and use that to set our idle timeout target (which as I mention, we often don't make it to). I can't recall off the top of my head whether that is MIN(client,server) or just taking the server value.

I can't get really excited about this whole area of inquiry - robustness requires that this all be event based instead of time based anyhow. I've never really seen much advantage in either side sending a time interval. If our header is malformed we should just stop sending it.
Comment 3 Boris Zbarsky [:bz] 2011-03-21 06:10:29 PDT
If servers and proxies actually ignore the Keep-Alive header (or even just ignore our broken version), then I agree we should stop sending it.  Want to write a patch?  ;)
Comment 4 Patrick McManus [:mcmanus] 2011-03-21 06:14:42 PDT
(In reply to comment #3)
>  Want to
> write a patch?  ;)

ok
Comment 5 Patrick McManus [:mcmanus] 2011-03-21 10:50:42 PDT
Created attachment 520681 [details] [diff] [review]
no-keep-alive-request-header v1
Comment 6 mnot 2011-03-21 17:14:59 PDT
Agreed that you shouldn't send it, esp. if it's inaccurate and if you're reasonably confident people don't use it. No need to emit the valid form unless there are real use cases.
Comment 7 Patrick McManus [:mcmanus] 2011-03-24 11:02:30 PDT
http://hg.mozilla.org/mozilla-central/rev/118451f8ec76
Comment 8 Joe Drew (not getting mail) 2011-03-24 17:34:52 PDT
Backed out on suspicion of causing tp4 regression: http://hg.mozilla.org/mozilla-central/rev/6d6811386347
Comment 9 Patrick McManus [:mcmanus] 2011-04-03 08:57:53 PDT
this is cleared of being the tp4 regression (it was 628561).. ready to push
again
Comment 10 Boris Zbarsky [:bz] 2011-04-04 22:43:16 PDT
Pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/6008b74c193f
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-04-05 11:50:24 PDT
http://hg.mozilla.org/mozilla-central/rev/6008b74c193f
Comment 12 Eric Shepherd [:sheppy] 2011-04-12 23:08:15 PDT
Turns out we didn't have that header documented anywhere although it was mentioned in passing in a few places in this article:

https://developer.mozilla.org/En/HTTP_access_control

I've cleared it out of there now.

This change is also now mentioned on Firefox 5 for developers, and on https://developer.mozilla.org/en/HTTP/Headers

Note You need to log in before you can comment on or make changes to this bug.