Last Comment Bug 603505 - pipeline project - pretest
: pipeline project - pretest
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: x86_64 Linux
: -- enhancement with 2 votes (vote)
: ---
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 562917 648091 602518 672958
Blocks: 603503 603506 pipelining-review
  Show dependency treegraph
 
Reported: 2010-10-11 16:58 PDT by Patrick McManus [:mcmanus]
Modified: 2012-11-30 08:59 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A sample server (6.62 KB, text/plain)
2010-10-20 12:41 PDT, Patrick McManus [:mcmanus]
no flags Details
pipeline-pretest v1 (43.69 KB, patch)
2010-10-20 13:01 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
a sample server (r2) (6.65 KB, text/plain)
2010-10-21 06:12 PDT, Patrick McManus [:mcmanus]
no flags Details
passed sanity test packet capture (14.49 KB, application/octet-stream)
2010-10-21 06:15 PDT, Patrick McManus [:mcmanus]
no flags Details
pipeline-pretest v2 (44.42 KB, patch)
2010-10-21 07:37 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
pipeline-pretest v3 (45.47 KB, patch)
2010-10-22 08:21 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
pipeline pretest v4 (44.53 KB, patch)
2010-11-12 08:16 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
pipeline pretest v5 (47.02 KB, patch)
2010-12-03 15:26 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
pipeline pretest v6 (46.45 KB, patch)
2011-01-12 17:10 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
pipeline pretest v7 (54.05 KB, patch)
2011-02-18 18:37 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
pipeline pretest v8 (56.28 KB, patch)
2011-06-26 22:09 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2010-10-11 16:58:20 PDT
a pretest http can be used to determine any basic pipelining problems with forward proxies on the main internet path for the UA. "detect and recover" is still needed at runtime for reverse facing proxies, but something more thorough can be used as a global litmus test when getting started.
Comment 1 Patrick McManus [:mcmanus] 2010-10-20 12:41:00 PDT
Created attachment 484784 [details]
A sample server

A client side sanity test is going to require a server to run against. This is a ~250 line proof of concept server in c++. The basic outline is pretty scalable but a number of tweaks should be made to it if is desired for a production scenario - the idea of including it here is just as a reference implication right now. An HTTP framework that operates on a transaction level isn't appropos for implementing this as you can see it plays fast and loose with connection semantics.
Comment 2 Patrick McManus [:mcmanus] 2010-10-20 13:01:11 PDT
Created attachment 484789 [details] [diff] [review]
pipeline-pretest v1

Adds a global "pipeline pre-test" to test out the network compatibility with pipelining between the client and whatever server is running the pretest-server (presumably pipeline.mozilla.com by default). The URL to test against and whether to use the test are defined by prefs:

pref("network.http.pipelining.testURL" , "http:/pipeline.ducksong.com:9111/sanity-test");
pref("network.http.pipelining.testURL.enabled" , true);

The test works like this 
 1] it opens a connection
 2] it sends down that connection a burst of 3 requests, each of which is labeled with a serial number
 3] Until the first response is seen it pipelines an additional 3 requests down the same pipe (with new serial numbers) every 33ms
 4] After the first response is received it continues to send a burst of 3 requests down the same pipe (with new serial numbers) every 33ms for the next 300 ms
 5] after the 300ms timer is complete it sends no more requests but waits to receive all the responses

Each response contains a header correlating it to the request that generated it. Both the client and server confirm that they are all received in the correct order and on the same connection and that a full window of transactions can be maintained.

The http responses are a variety of 200, 100 + 200, and 304 responses - designed to give intermdiaries with a loose sense of framing fits. The server does not generate any responses until at least 3 requests have arrived - validating that there is not an intermediary breaking the pipeline. 

The content length of each response generally varies to match the low 5 bits of the serial number, so it can be verified on the client side, except for the first response which carries of list of pipeline-blacklist host names to be used by bug 603506.

There are some open issues - primarily what server to run against, when to run the test (right now it is on every startup, which isn't right)
Comment 3 Jason Duell [:jduell] (needinfo me) 2010-10-21 05:17:24 PDT
Any reason why we can't extend httpd.js to test pipelining?  Ought to be easier to write/maintain than a C++ server.   Or maybe not--just want to make sure it's on your radar.
Comment 4 Patrick McManus [:mcmanus] 2010-10-21 05:54:36 PDT
(In reply to comment #3)
> Any reason why we can't extend httpd.js to test pipelining?  Ought to be easier

Well, the real answer is "I don't know - the server here is just a PoC".. but my inclination on httpd.js is no..

 * need to work on the connection level not the transactions level.. can httpd.js let me inspect transaction n+3 before responding to n while still correlating that they are on same connection? At first glance, I don't think so - but I'm not certain. This is the fundamental problem with reusing any framework I've stumbled across.

  * the "test" in question is a runtime topography test, not a quality-assurance test - is httpd.js scalable to the demands of all those parallel users?  (I don't know the answer) Other than some RAM and silly copies in the c++ version that could be fixed with a little effort, the c++ version is very light weight and other than serialization at the OS level on an accept() call, lockless but still multi-threaded.

  * the poc server is just 250 lines long and has no dependencies, it doesn't even speak (nor need to) full http. 

but then again, while it has some good qualities, the crude thing that is there is almost certainly just a PoC. I'm torn on what to really do both here and for 603513.. for the moment I'm happy to focus on whether or not the test being performed integrates ok into FF and accomplishes what it sets out to do (i.e. identify obstructed paths).
Comment 5 Patrick McManus [:mcmanus] 2010-10-21 06:12:17 PDT
Created attachment 485029 [details]
a sample server (r2)

the server should not respond until req 4 is seen (not req 3 as previously written) - this ensures a significantly more challenging test as the outstanding requests now cross different batches (and most certainly different tcp segments).
Comment 6 Patrick McManus [:mcmanus] 2010-10-21 06:15:31 PDT
Created attachment 485030 [details]
passed sanity test packet capture

If anyone is interested - this is a capture of what the test ends up looking like over a normal broadband connection.. ~44 transactions taking just 15KB in about 400ms including all network overhead.. all done in bg on ff. Nice overlap in sending from both directions at the same time which is what we want to see to feel confident everything is working.
Comment 7 Patrick McManus [:mcmanus] 2010-10-21 07:37:00 PDT
Created attachment 485041 [details] [diff] [review]
pipeline-pretest v2

update this to actually verify the response bodies and drain the response queue
Comment 8 Honza Bambas (:mayhemer) 2010-10-21 07:45:14 PDT
Jeff, I remember you wanted to add support for pipelining to httpd.js in the past, is it still actual?  You could cooperate with Patrick on making that change.  He seems to have a good picture of what's needed for that.
Comment 9 Patrick McManus [:mcmanus] 2010-10-22 05:39:35 PDT
mnot sends along this: http://gist.github.com/569672 .. which is in twisted python and is a nice illustration of processing a pipeline of requests ahead of generating any responses..
Comment 10 Patrick McManus [:mcmanus] 2010-10-22 08:21:42 PDT
Created attachment 485293 [details] [diff] [review]
pipeline-pretest v3

update to implement weakref to correctly be an observer
Comment 11 Patrick McManus [:mcmanus] 2010-11-12 08:16:58 PST
Created attachment 490100 [details] [diff] [review]
pipeline pretest v4

fix compiler failure under different build opts
Comment 12 Patrick McManus [:mcmanus] 2010-12-03 15:26:47 PST
Created attachment 495142 [details] [diff] [review]
pipeline pretest v5

update bitrot, confrom better to style guide, updates based on experience (i.e. bugs and tweaks), etc..
Comment 13 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2010-12-29 15:17:33 PST
Can we merge the pipelining pretest with captive portal detection and/or build pipelining pretest on top of captive portal detection?

I added a dependency on bug 340548. Either we could disable pipelining when we have detected a captive portal or else we need to retry the pipelining pretest after we've gotten past the captive portal.
Comment 14 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2010-12-29 15:24:06 PST
I meant 562917. Sorry for the spam.
Comment 15 Patrick McManus [:mcmanus] 2011-01-12 17:10:23 PST
Created attachment 503352 [details] [diff] [review]
pipeline pretest v6

bitrot update
Comment 16 Patrick McManus [:mcmanus] 2011-02-18 18:37:01 PST
Created attachment 513671 [details] [diff] [review]
pipeline pretest v7

bitrot due to syn retry deps.. putting feedback? instead of r? on this because we need to reoslve the hosting considerations..
Comment 17 Patrick McManus [:mcmanus] 2011-06-26 22:09:43 PDT
Created attachment 542083 [details] [diff] [review]
pipeline pretest v8

update bitrot to reflect larch
Comment 18 Honza Bambas (:mayhemer) 2011-07-20 14:54:36 PDT
Comment on attachment 542083 [details] [diff] [review]
pipeline pretest v8

Review of attachment 542083 [details] [diff] [review]:
-----------------------------------------------------------------

If this is actual:

"The test works like this
1] it opens a connection
2] it sends down that connection a burst of 3 requests, each of which is
labeled with a serial number
3] Until the first response is seen it pipelines an additional 3 requests down
the same pipe (with new serial numbers) every 33ms
4] After the first response is received it continues to send a burst of 3
requests down the same pipe (with new serial numbers) every 33ms for the next
300 ms
5] after the 300ms timer is complete it sends no more requests but waits to
receive all the responses"

..then you really don't need so much of a new code and modifications for it.

Let me outline my idea:

In the T+S patch I mentioned we should have a way (an API, actually) to explicitly set a pipelining class on an http channel (bug 672958).  As you want to use just a single connection for the 9 pretest requests then we can use that API and set a class "pretest" to force just a single connection for the traffic (this class can also be used to switch to a "testing" behavior, if needed).  Then we can use 3 nsHttpChannels to send the first burst, and have a listener (here actually the "tester") on all of them, that will monitor progress and process remaining 2 burst and finally collect results.  I believe that can be done even only with JavaScript!


According the server: I would like to use something existing.  PHP will probably not be enough (however my knowledge is not that deep to say certainly) to wait for 3 requests to come to the server and only then send out responses.  If it gets confirmed, we might want to find or write a simple Apache mod.  I would rather put effort this way rather then inventing a new server code with potential security holes running on public production machine.


Let's start talking about how to implement the pretest a simpler way then this patch does.  For now I'm dropping the review flag.  Patrick, next time we should talk first before you write so much code..
Comment 19 Patrick McManus [:mcmanus] 2011-12-13 07:38:47 PST
I've decided to put this patch into cold storage for a while until a demonstrated need for it is established. I do this because:

* experience with mobile pipelining (in FF and other browsers) indicates to me this is not a pervasive problem.
* any issues the pre-test discovers should be discoverable through other means
* privacy concerns
* services concerns and lack of progress on server side production implementation

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