User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.1) Gecko/2008071717 Firefox/3.0.1 Build Identifier: fennec m5 I have observed that when fetching a page with max connection of 1, I would never see pipelining while the base HTML document was being transferred. I thought for a bit this was 364315 (speculative loader) but it applies in the case of javascript as well as images, et al.. and that bug does not apply to fetching js. Furthermore, if I have 2 connections the javascript is indeed fetched in parallel with the html page - so it is not a case of not scheduling the transaction, it just won't pipeline it if a free connection is unavailable. This generalizes to any case needing > N objects over N connections. The first N go in parallel, but no pipelining occurs until one of those first connections frees up. But I do see pipelining, just never against the first transaction on a connection. Which brings me to the meat of this issue. I've dug around and I'm looking for clarifications on what I think is going on, roughly speaking: .. New transactions come into nsHttpConnectionMgr::ProcessNewTransaction() and the manager tries to find a connection for them to use.. this might be an idle persistent connection, or it might be a new tcp session. this allocation is subject to per-server and UA-wide limits. If it finds a connection the http request is dispatched to that connection and sent right out.. if it can't find one (probably due to those limits) the transaction is put on the pending queue in memory. when a connection finishes its scheduled work, it goes through the reclaim logic, and then a new transaction is plucked from the pending queue (if any) and dispatched again (either reusing that connection in the case of persistent conns, or using a new connection). Pipelining is cleverly wedged in here through the "dispatch" logic - which basically says when sending its request, if there are more transactions queued up (subject to pipelining suitability of course) then add some of those transactions (up to the number allowed by the maxpipelinedrequests preference) to this connection too. When the connection is activated there may be multiple requests written out all at the same time in a pipelined burst. ... this means that requests are never pipelined at the rate they are identifed to ::ProcessNewTransaction(). PNT always sends them out right way or adds latency by placing the in pending queue. The reclaim logic then grabs as many as it is allowed from the queue the next time around and sends them out in a burst. This creates a couple problems * the aforementioned problem where the first transaction on an idle connection (dispatched directly from PNT()) never has anything pipelined against it - and this likely includes key components such as js and css against base HTML. * The burstiness of the request bundling leads to additional transfer latency as upstream bandwidth could be exhausted and network queuing delays kick in - especially true on mobile where upstream bandwidths can be stingy at best. Burstiness will also interfere with any strategies we may want to implement to manage the windows on different connections to dynamically balance their priorities.  Right now the flow looks like OUT: > >> >>>> IN: <<<< <<<<<<<<<< <<<<<<<<<<<<<<<<<<<< Where it could look like OUT: > > > > > > > IN: <<<< <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< The latter finishes quicker and is smoother - both desirable properties. I don't have enough of a handle on the code yet to even suggest how to fix it - it certainly looks like connection::Activate() wants to have everything queued on it and then run to completion before having new transactions enqueued.. but I'm sure its not a huge deal. Reproducible: Always

Are you using the network.http.pipelining and network.http.proxy.pipelining configuration variables (in about:config) ? Without them pipelining is not used at all (their default is false !). Also note that it also depends on the webserver that is being used : see <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpConnection.cpp#231>. IIS 4 and 5 will never use pipelining !

hi Jo - thanks for the comment. Please note that the description shows that pipelining is configured and does indeed occur, it just only happens in limited circumstances (never against an in use connection, the pipeline is always filled in a burst on a new or idle connection.) It's not a defect, but rather an enhancement opportunity I came across when looking at fennec behavior on mobile networks.

the above analysis in the description is a little bit off the mark - It describes the only way to start a pipelined xaction as through BuildPipeline() (via dispatch) which is executed on idle connections (either idle pconns or new connections) and results in bursts of requests. There is another path - via addtransactiontopipeline().. this fx only ever adds 1 transaction and it is called from the pipeline code itself after it one outstanding pipelined xact has completed.. so this is really meant to replace the one just completed. This new path does add a little smoothing to the process, but there is still un-necessary latency in the path (i.e. that transaction need not wait for completion of an outstanding one if the quota is not exhausted)

Document showing impact of patch as a packet time series, and also as a cumulative data transfer vs time.

From patch description: Shown to improve page load times 10% on mobile networks for pages with sufficient pipelining opportunity. * cache supports-pipelining detection on connection entry info * reschedule queued transactions onto existing connection after receipt of pipeline-capable headers instead of waiting for whole transaction to complete * dispatch pipelined transactions immediately instead of waiting for connection to recycle * create pipeline c++ objects a little more aggressively before even if we only put one transaction onto them at first (i.e. don't actually perform HTTP pipelining), as this preserves the ability to fill their pipeline if response headers indicate it is ok and new requests are introduced, without waiting for the whole transaction to complete. This patch does not change the pipelining criteria for any given transaction or server combination, it simply decreases the latency for pipelining eligible transactions.

Comment on attachment 334620 [details] [diff] [review] patch for functionality addressed in comment 5 please don't use tabs, use 4 spaces

Comment on attachment 334620 [details] [diff] [review] patch for functionality addressed in comment 5 This really needs Christian to look at it.

When landing this patch we ought to also be aware of the side effects that will occur. There is some experience from past attempts at doing this that we shouldn't ignore. https://bugzilla.mozilla.org/show_bug.cgi?id=264354#c3 https://bugzilla.mozilla.org/show_bug.cgi?id=264354#c5 https://bugzilla.mozilla.org/show_bug.cgi?id=264354#c7 Are a starting point for evaluating the problems and overall effectiveness of the patch. Do with think the web is in much better shape for landing this this patch now than it was in 2004 or before? We need some depth testing to help figure that out. I get about 35 Open/Dup bugs with "Pipelining" in the title with the simple query and 77 over all bugs with the query https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=pipelining The bugs breakdown into three major areas 100% cpu use with pipelining turned on sites don't load with pipelining turned on sites load, but are broken with pipelining turned on. We should try to pull together a test plan that outlines each of these areas and problems found from these past attempts at turning on pipelining. That way we go into the landing of the patch with eyes wide open. If opera and other mobile browsers are using pipelining thats not a big indicator that its the right move to be making. There are reasons behind the low market share and use of these browsers, and compatibility is one of the reasons that is pretty high on the list. We need some new testing data to make sure we aren't following them down a path of broken websites and introducing some 100% cpu use performance problems when we hit some sites and servers that are broken...

(In reply to comment #9) > When landing this patch we ought to also be aware of the side effects that will > occur. There is some experience from past attempts at doing this that we > shouldn't ignore. > Hi Chris, Thanks for the comment. I think there might be a misunderstanding about the impact of this change. The patch does not change the circumstances under which firefox decides to pipeline a request. The quotas, preferences, and heuristics (including screens for known broken server types, etc..) that applied before, still apply after this change. The default preferences are unmodified. What is changed are some implementation details about how the pipeline itself is implemented. They reduce delay inside the browser and create less bunching of the requests themselves which uses the network more effectively. There are indeed some cases where these details were so significant that they previously prevented pipelining from happening at all in the baseline browser (put more accurately, firefox was willing to pipeline but didn't get around to it fast enough before there was nothing to pipeline against). But there is no reason to think those requests are more (or less) likely to have interop problems than the ones that did go out successfully under the same config. with that said, I don't really think the "turn pipelining on by default" bug is germane to this change.

Hi Patrick, Thanks for clarifying. https://wiki.mozilla.org/Platform/2008-09-09#Mobile_1.9.1_Update and some comments in the meeting and IRC from pav were implying this was a critical patch on the path for trying to turn on pipelining in the next few days so it could make fx 3.1/mozilla-1.9.1. I guess my comments belong in another bug if we are thinking about trying to do that on a short timeline. sorry about that...

chofmann: all of my comments, including the ones on that wiki page, are for fennec, which is a separate application than firefox. pipelining is already enabled by default in fennec. anyways, if you have further concerns, i suggest taking them to the mobile newsgroup.

bz, brendan: any idea who can review this? this needs to land by tuesday

darin and maybe biesi are the only people who can review this without putting in a bunch of time learning how our http impl works. Yes, it does suck. No, I'm not doing another bfcache "pretend to review code that I don't really understand because someone feels like he's under time pressure" here.

I'm putting this on my list, and when I actually get familiar enough with the network stack to review things, I'll review Patrick's patch. Of course, if anyone else can review it in the mean time, feel free to...

Going to review this.

The patch looks good to me at the first look. I can confirm you are on the right way as I have already made a very similar patch bug 422978 attachment 314660 [details] [diff] [review] that does almost the same as your patch does. Only my patch is much simpler. Patrick, it is pain to review your patch, can you please re-submit it using proper indention (4 spaces instead of tabs) and larger context? We use 8 lines context. Thanks. ...hmmm, I guess that if I move 'gHttpHandler->ConnMgr()->AddTransactionToPipeline(this)' from nsHttpPipeline::WriteSegments to nsHttpPipeline::OnHeadersAvailable then the patches will be functionally identical... Going to check this. Patrick please take a look at my patch as well.

fix a couple of test failures and output of jst-review.pl

Are these patches ready for review? Ask for it if/when you need it.

Comment on attachment 474681 [details] [diff] [review] patch to smooth out burstiness of pipelining Honza allegedly has similar patch waiting. Letting him do the review.

Update patch to incorporate TakeHttpConnection() logic of 595316 - I think have convinced myself the trunk is not vulnerable to the 595316 bug without the rest of this patch.

Patrick, thanks for updated patches, I'll get to them hopefully next week to review.

update to fix two more instances of the type of problem from comment 22, where nsHttpConnection code was just casting nsAHttpTransaction objects to nsHttpTransaction because it arbitrarily knew it to be correct. This patch didn't do previously anything to invalidate that assumption (like it had with c22), but there really doesn't seem to be any point in having the abstract class if you're just going to cast it away when you're not even in the heirarchy. So this patch now provides methods on the base object for resolving what you need without the cast.

You had to leave the patch for bug 595316 there (and the bug open). Adding more changes, that are not related to scope of this bug makes the review harder... Of course, patch for this bug could be easily based on patch from bug 595316.

(In reply to comment #25) > You had to leave the patch for bug 595316 there (and the bug open). Adding > more changes, that are not related to scope of this bug makes the review > harder... Of course, patch for this bug could be easily based on patch from > bug 595316. it turns out that the base patch here is what made 595316 truly crashable which is why I rolled it in here. It might be harder, but they really should not be separated.

Sorry, I had to think before I have sent my last comment. I wanted actually to say: please split the patches again, it makes review simpler, make this bug dep on bug 595316.

Remove the bits in the patch that were really about 595316 and make this bug depend on that one.

Comment on attachment 480621 [details] [diff] [review] Smooth out pipeline burstiness Yes, it's me again :) >diff --git a/netwerk/protocol/http/nsAHttpTransaction.h b/netwerk/protocol/http/nsAHttpTransaction.h >--- a/netwerk/protocol/http/nsAHttpTransaction.h >+++ b/netwerk/protocol/http/nsAHttpTransaction.h >@@ -86,30 +86,41 @@ public: > // called to close the transaction > virtual void Close(nsresult reason) = 0; > > // called to indicate a failure at the SSL setup level > virtual void SetSSLConnectFailed() = 0; > > // called to retrieve the request headers of the transaction > virtual nsHttpRequestHead *RequestHead() = 0; >+ >+ // called to add a sub-transaction in the case of pipelined transactions >+ // classes that do not implement sub transactions >+ // return NS_ERROR_NOT_IMPLEMENTED >+ virtual nsresult AddTransaction(nsAHttpTransaction *transaction) = 0; >+ >+ // called to count the number of already active sub transactions >+ virtual PRInt32 CountOutstandingSubTransactions() = 0; PRUint32? >+ Nit: empty line. > }; >diff --git a/netwerk/protocol/http/nsHttpConnection.h b/netwerk/protocol/http/nsHttpConnection.h >--- a/netwerk/protocol/http/nsHttpConnection.h >+++ b/netwerk/protocol/http/nsHttpConnection.h >@@ -34,19 +34,21 @@ > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #ifndef nsHttpConnection_h__ > #define nsHttpConnection_h__ > >+class nsHttpRequestHead; >+class nsHttpResponseHead; >+ After the includes, please. > #include "nsHttp.h" > #include "nsHttpConnectionInfo.h" >-#include "nsAHttpConnection.h" > #include "nsAHttpTransaction.h" > #include "nsXPIDLString.h" > #include "nsCOMPtr.h" > #include "prlock.h" > > #include "nsIStreamListener.h" > #include "nsISocketTransport.h" > #include "nsIAsyncInputStream.h" >diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/nsHttpConnectionMgr.cpp >--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp >+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp >@@ -227,37 +227,50 @@ nsHttpConnectionMgr::GetSocketThreadTarg > nsAutoMonitor mon(mMonitor); > NS_IF_ADDREF(*target = mSocketThreadTarget); > return NS_OK; > } > > void > nsHttpConnectionMgr::AddTransactionToPipeline(nsHttpPipeline *pipeline) > { >+ /* called on an existing pipeline anytime we might add more data to an >+ existing pipeline such as when a transaction completes (and >+ therefore the quota has new room), or when we receive headers which >+ might change our view of pipelining */ >+ > LOG(("nsHttpConnectionMgr::AddTransactionToPipeline [pipeline=%x]\n", pipeline)); >- >+ > NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread"); >- >+ PRInt32 avail = mMaxPipelinedRequests - >+ pipeline->CountOutstandingSubTransactions(); >+ > nsRefPtr<nsHttpConnectionInfo> ci; > pipeline->GetConnectionInfo(getter_AddRefs(ci)); >- if (ci) { >+ if (ci && avail && ci->SupportsPipelining()) { I know this isn't your code, but feel like converting this to early-return style? if (!avail) return; nsRefPtr<nsHttpConnectionInfo> ci; pipeline->GetConnectionInfo(getter_AddRefs(ci)); if (!ci || !ci->SupportsPipelining()) return; > nsCStringKey key(ci->HashKey()); > nsConnectionEntry *ent = (nsConnectionEntry *) mCT.Get(&key); > if (ent) { > // search for another request to pipeline... > PRInt32 i, count = ent->mPendingQ.Length(); >- for (i=0; i<count; ++i) { >- nsHttpTransaction *trans = ent->mPendingQ[i]; >+ for (i = 0; i < count ; i++) { No space before ";", keep ++i. >+ nsHttpTransaction *trans = >+ (nsHttpTransaction *) ent->mPendingQ[i]; > if (trans->Caps() & NS_HTTP_ALLOW_PIPELINING) { > pipeline->AddTransaction(trans); > > // remove transaction from pending queue > ent->mPendingQ.RemoveElementAt(i); >+ // adjust iterator to reflect coalesced queue >+ count--; >+ i--; --count, --i. >+ > NS_RELEASE(trans); >- break; >+ if (--avail == 0) >+ break; > } > } > } > } > } > > nsresult > nsHttpConnectionMgr::ReclaimConnection(nsHttpConnection *conn) >@@ -454,16 +467,84 @@ nsHttpConnectionMgr::ProcessPendingQForE > > NS_RELEASE(conn); > return PR_TRUE; > } > } > return PR_FALSE; > } > >+PRBool >+nsHttpConnectionMgr::ProcessPipelinePendingQForEntry(nsConnectionEntry *ent) >+{ >+ LOG(("nsHttpConnectionMgr::ProcessPipelinePendingQForEntry [ci=%s]\n", >+ ent->mConnInfo->HashKey().get())); >+ >+ NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread"); >+ >+ if (mMaxPipelinedRequests < 2) >+ return PR_FALSE; >+ >+ PRInt32 activeCount = ent->mActiveConns.Length(); >+ PRInt32 originalPendingCount = ent->mPendingQ.Length(); >+ PRInt32 pendingCount = originalPendingCount; >+ PRInt32 j = 0; >+ >+ for (PRInt32 i = 0; (i < activeCount) && (j < pendingCount); i++) { Drop the (). >+ No empty line. >+ nsHttpConnection *conn = (nsHttpConnection *) ent->mActiveConns[i]; static_cast >+ >+ if (!conn->SupportsPipelining()) >+ continue; >+ >+ nsAHttpTransaction *activeTrans = conn->Transaction(); >+ if (!activeTrans) >+ continue; >+ >+ nsresult rv = NS_OK; >+ PRInt32 avail = mMaxPipelinedRequests - >+ activeTrans->CountOutstandingSubTransactions(); >+ >+ while (NS_SUCCEEDED(rv) && (avail > 0) && (j < pendingCount)) less (). >+ { { on the previous line >+ nsHttpTransaction *trans = (nsHttpTransaction *) ent->mPendingQ[j]; static_cast. >+ >+ if (trans->Caps() & NS_HTTP_ALLOW_PIPELINING) { >+ rv = activeTrans->AddTransaction(trans); >+ if (NS_SUCCEEDED(rv)) { >+ // remove transaction from pending queue >+ ent->mPendingQ.RemoveElementAt(j); >+ >+ // adjust iterator to reflect coalesced queue >+ pendingCount--; >+ avail--; >+ NS_RELEASE(trans); >+ } >+ } >+ else >+ // skip over this one >+ j++; >+ } >+ } --var, ++var >+ return (originalPendingCount == pendingCount) ? PR_FALSE : PR_TRUE; return originalPendingCount != pendingCount >+} >+ >+ >+PRBool >+nsHttpConnectionMgr::ProcessPipelinePendingQForCI(nsHttpConnectionInfo *ci) >+{ >+ NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread"); >+ >+ nsCStringKey key(ci->HashKey()); >+ nsConnectionEntry *ent = (nsConnectionEntry *) mCT.Get(&key); Again. >+ >+ return ent && ProcessPipelinePendingQForEntry(ent); Only one space after &&. >+} >+ >+ > // we're at the active connection limit if any one of the following conditions is true: > // (1) at max-connections > // (2) keep-alive enabled and at max-persistent-connections-per-server/proxy > // (3) keep-alive disabled and at max-connections-per-server > PRBool > nsHttpConnectionMgr::AtActiveConnectionLimit(nsConnectionEntry *ent, PRUint8 caps) > { > nsHttpConnectionInfo *ci = ent->mConnInfo; >@@ -618,50 +701,58 @@ nsHttpConnectionMgr::DispatchTransaction > return rv; > } > > PRBool > nsHttpConnectionMgr::BuildPipeline(nsConnectionEntry *ent, > nsAHttpTransaction *firstTrans, > nsHttpPipeline **result) > { >+ NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread"); >+ > if (mMaxPipelinedRequests < 2) > return PR_FALSE; > >- nsHttpPipeline *pipeline = nsnull; >- nsHttpTransaction *trans; >+ /* form a pipeline here even if nothing is pending so that we >+ can stream-feed it as new transactions arrive */ > >- PRUint32 i = 0, numAdded = 0; >- while (i < ent->mPendingQ.Length()) { >- trans = ent->mPendingQ[i]; >- if (trans->Caps() & NS_HTTP_ALLOW_PIPELINING) { >- if (numAdded == 0) { >- pipeline = new nsHttpPipeline; >- if (!pipeline) >- return PR_FALSE; >- pipeline->AddTransaction(firstTrans); >- numAdded = 1; >+ nsHttpPipeline *pipeline = new nsHttpPipeline; >+ if (!pipeline) >+ return PR_FALSE; Drop this check, new can't fail. >+ >+ /* the first transaction can go in unconditionally - 1 transaction >+ on a nsHttpPipeline object is not a real HTTP pipeline */ >+ >+ PRInt32 numAdded = 1; >+ pipeline->AddTransaction(firstTrans); >+ >+ if (ent->mConnInfo->SupportsPipelining()) { >+ PRUint32 i = 0; >+ nsHttpTransaction *trans; >+ >+ while (i < ent->mPendingQ.Length()) { >+ trans = (nsHttpTransaction *) ent->mPendingQ[i]; >+ if (trans->Caps() & NS_HTTP_ALLOW_PIPELINING) { >+ pipeline->AddTransaction(trans); >+ >+ // remove transaction from pending queue >+ ent->mPendingQ.RemoveElementAt(i); >+ NS_RELEASE(trans); >+ >+ if (++numAdded == mMaxPipelinedRequests) >+ break; > } >- pipeline->AddTransaction(trans); >- >- // remove transaction from pending queue >- ent->mPendingQ.RemoveElementAt(i); >- NS_RELEASE(trans); >- >- if (++numAdded == mMaxPipelinedRequests) >- break; >+ else >+ ++i; // skip to next pending transaction } else { ++i; } (Always brace else if you brace if.) > } >- else >- ++i; // skip to next pending transaction > } > >- if (numAdded == 0) >- return PR_FALSE; >- >- LOG((" pipelined %u transactions\n", numAdded)); >+ if (numAdded > 1) >+ LOG((" pipelined %u transactions\n", numAdded)); >+ > NS_ADDREF(*result = pipeline); > return PR_TRUE; > } > > nsresult > nsHttpConnectionMgr::ProcessNewTransaction(nsHttpTransaction *trans) > { > // since "adds" and "cancels" are processed asynchronously and because

> >+ count--; > >+ i--; > > --count, --i. prefix vs postfix increments should be the same efficiency for built-in types, but I suppose it's a good habit to use prefix style since it may be more efficient for complex types: http://www.parashift.com/c++-faq-lite/operator-overloading.html#faq-13.15 > >+ for (PRInt32 i = 0; (i < activeCount) && (j < pendingCount); i++) { > > Drop the (). I don't have a problem with making precedence clearer to the reader with "extra" parens. > } else { > ++i; > } > > (Always brace else if you brace if.) Well, but he didn't brace the if :) I know some of us feel we should always use braces when you've got an if..else, and reserve no braces for one-line if statements (the style guide says to use braces even then, which is widely ignored). I don't care much, so long as there's only one line per if and else (including comments). All of ms2ger's other comments are correct: thanks for the code style policing! :)

update bitrot

update for bitrot, coding style, bug with ssl proxy connect, bug with status waiting for events, restart deep pipeline safe events

update bitrot

Comment on attachment 537844 [details] [diff] [review] v9 (by Patrick McManus) merged Review of attachment 537844 [details] [diff] [review]: ----------------------------------------------------------------- Please fix the patch comment before landing to "Bug 447866 - http pipelining is bursty, r=honzab" or something like that. r-, see the comments: ::: netwerk/protocol/http/nsAHttpTransaction.h @@ +101,5 @@ > + // return NS_ERROR_NOT_IMPLEMENTED > + virtual nsresult AddTransaction(nsAHttpTransaction *transaction) = 0; > + > + // called to count the number of already active sub transactions > + virtual PRInt32 CountOutstandingSubTransactions() = 0; I would rather change this method to return how many transactions can be added to the pipeline. On all places this method is used you subtract this method result from mMaxPipelinedRequest to figure out how many you can add. Turning this method to do it internally is better. Let's name it PipelineAvailable or whatever name you feel is better. Also implementation of CountOutstandingSubTransactions for nsHttpTransaction doesn't make sense at all. If we turn it the way I suggest it may return 0, what makes sense. Result should be unsigned int, best PRUint16 as mMaxPipelinedRequests is. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +4078,5 @@ > // on proxy errors, try to failover > if (mConnectionInfo->ProxyInfo() && > (mStatus == NS_ERROR_PROXY_CONNECTION_REFUSED || > mStatus == NS_ERROR_UNKNOWN_PROXY_HOST || > + mStatus == NS_ERROR_NET_RESET || Why this change? How is this related to pipelining optimization? Is this related to changes with transaction restart bellow? Please add comments to the code for changes like these. ::: netwerk/protocol/http/nsHttpConnection.h @@ +116,5 @@ > void DropTransport() { DontReuse(); mSocketTransport = 0; } > > + PRBool ForceReadEvents() > + { > + return mProxyConnectStream != nsnull; Fixed from !!mSSLProxyConnectStream during merge. Please don't use !! for pointer to bool conversion. ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +333,2 @@ > LOG(("nsHttpConnectionMgr::AddTransactionToPipeline [pipeline=%x]\n", pipeline)); > + Remove this white space change please. @@ +339,3 @@ > nsRefPtr<nsHttpConnectionInfo> ci; > pipeline->GetConnectionInfo(getter_AddRefs(ci)); > + if (ci && avail && ci->SupportsPipelining()) { Better do avail > 0 in case mMaxPipelinedRequests changed by pref manipulation? Another argument to change CountOutstandingSubTransactions method to something different. @@ +343,5 @@ > nsConnectionEntry *ent = (nsConnectionEntry *) mCT.Get(&key); > if (ent) { > // search for another request to pipeline... > PRInt32 i, count = ent->mPendingQ.Length(); > + for (i = 0; i < count; i++) { Rather then i++ here do it in else block of |if (trans->Caps() & NS_HTTP_ALLOW_PIPELINING)| ? This i++/i-- is a bit weird. @@ +345,5 @@ > // search for another request to pipeline... > PRInt32 i, count = ent->mPendingQ.Length(); > + for (i = 0; i < count; i++) { > + nsHttpTransaction *trans = > + (nsHttpTransaction *) ent->mPendingQ[i]; You don't need this cast. mPendingQ items are of nsHttpTransaction type already. @@ +350,2 @@ > if (trans->Caps() & NS_HTTP_ALLOW_PIPELINING) { > pipeline->AddTransaction(trans); What if this fails? It might when pipeline is not a pipeline but a transaction here. @@ +654,5 @@ > + > + PRInt32 activeCount = ent->mActiveConns.Length(); > + PRInt32 originalPendingCount = ent->mPendingQ.Length(); > + PRInt32 pendingCount = originalPendingCount; > + PRInt32 j = 0; Maybe rename this to pendingIndex? Also i bellow to connectionIndex or activeIndex. The code is then more readable. All these should be PRUint32 (unsigned). @@ +656,5 @@ > + PRInt32 originalPendingCount = ent->mPendingQ.Length(); > + PRInt32 pendingCount = originalPendingCount; > + PRInt32 j = 0; > + > + for (PRInt32 i = 0; (i < activeCount) && (j < pendingCount); i++) { i should be PRUint32. @@ +658,5 @@ > + PRInt32 j = 0; > + > + for (PRInt32 i = 0; (i < activeCount) && (j < pendingCount); i++) { > + nsHttpConnection *conn = > + static_cast<nsHttpConnection *>(ent->mActiveConns[i]); No need for cast here. @@ +670,5 @@ > + > + nsresult rv = NS_OK; > + PRInt32 avail = mMaxPipelinedRequests - > + activeTrans->CountOutstandingSubTransactions(); > + What if the transaction here really is a transaction and not a pipeline? Avail will always be mMaxPipelinedRequests - 1, really a nonsense to me. Another argument to change the method. @@ +673,5 @@ > + activeTrans->CountOutstandingSubTransactions(); > + > + while (NS_SUCCEEDED(rv) && (avail > 0) && (j < pendingCount)) { > + nsHttpTransaction *trans = > + static_cast<nsHttpTransaction *>(ent->mPendingQ[j]); Again, no need for this cast. @@ +947,2 @@ > > + nsHttpPipeline *pipeline = new nsHttpPipeline(); I see a major issue here: you have removed check for NS_HTTP_ALLOW_PIPELINING for the first transaction. If it is not allowed, it is still added to a pipeline. You can admit that just adding a first transaction to the pipeline is not actual pipelining. The problem is in restarting. IsReused is implemented a different way in nsHttpPipeline then in nsHttpConnection. We can end up restarting a POST (or any non-idempotent method) with this code even though we shouldn't. We must reconsider nsHttpPipeline::IsReused implementation before we allow this patch to land or create and insert the pipeline object later. Have to think about this. @@ +949,5 @@ > + > + /* the first transaction can go in unconditionally - 1 transaction > + on a nsHttpPipeline object is not a real HTTP pipeline */ > + > + PRInt32 numAdded = 1; Should be PRUint16 @@ +957,5 @@ > + PRUint32 i = 0; > + nsHttpTransaction *trans; > + > + while (i < ent->mPendingQ.Length()) { > + trans = (nsHttpTransaction *) ent->mPendingQ[i]; No need for cast. ::: netwerk/protocol/http/nsHttpPipeline.cpp @@ +186,5 @@ > + requestHead, > + responseHead, > + reset); > + > + if ((!pipeliningBefore) && ci->SupportsPipelining()) no need for () around !pipeliningBefore @@ +412,3 @@ > nsAHttpTransaction *trans = Request(0); > + if (!trans) > + trans = Response(0); Why wasn't this needed before? @@ +432,5 @@ > > nsAHttpTransaction *trans; > switch (status) { > case NS_NET_STATUS_RECEIVING_FROM: > + case nsISocketTransport::STATUS_WAITING_FOR: Why this change? I assume the forwarding might be broken anyway. Also, there is NS_NET_STATUS_WAITING_FOR for this constant. @@ +548,5 @@ > nsAHttpTransaction *trans; > nsresult rv; > > trans = Response(0); > + if (!trans && mRequestQ.Length()> 0 && mConnection->ForceReadEvents()) { "Length()>" space before > please. @@ +549,5 @@ > nsresult rv; > > trans = Response(0); > + if (!trans && mRequestQ.Length()> 0 && mConnection->ForceReadEvents()) { > + LOG(("nsHttpPipeline::WriteSegments [this=%x] Forced Delegation\n", %p please @@ +553,5 @@ > + LOG(("nsHttpPipeline::WriteSegments [this=%x] Forced Delegation\n", > + this)); > + trans = Request(0); > + } > + Why this change? Why wasn't this needed before? Please explain and also add a comment to the code. @@ +627,5 @@ > + // if the response has not started to arrive and the reason > + // for failure is innocuous (e.g. not an SSL error) > + if (!mResponseIsPartial && > + (reason == NS_ERROR_NET_RESET || reason == NS_OK || > + reason == NS_BASE_STREAM_CLOSED)) { Before this change we force restart whenever the response was complete. After this change we force restart only when the response is complete and the connection is closed with one of NS_ERROR_NET_RESET, NS_OK, NS_BASE_STREAM_CLOSED errors. I am not sure I understand the motivations here, but I would do this backwards... if the response has been received only partially (also means that mReceivedData is set in the transaction, right?) we should not allow restart (i.e. we should change the error to something different from NET_RESET). But prevention is already ensured by the logic in nsHttpTransaction::Close. So I'm not convinced about this change, but please feel free to convince me.

Honza, I appreciate the feedback. Most of it I simply have applied, and a couple of your questions I need to research before replying because these patches are many months old and I did not document the motivation of every change well enough. Below are a couple issues that don't fall into either of the above categories, I wanted to document them now so they didn't get lost. > @@ +432,5 @@ > > > > nsAHttpTransaction *trans; > > switch (status) { > > case NS_NET_STATUS_RECEIVING_FROM: > > + case nsISocketTransport::STATUS_WAITING_FOR: > > Why this change? I assume the forwarding might be broken anyway. > generated by http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnection.cpp#737 consumed by http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#385 which in turn generates NS_HTTP_ACTIVITY_SUBTYPE_REQUEST_BODY_SENT, which in turn is used in a bunch of HUD tests. Its clear to me the target of that event is Response(0), as the transaction is moved to the response q after the request is sent (which is what the event is about) - thus the change. And yes, that's probably broken with the current m-c pipeline implementation. I believe there are several details that aren't robust with it. It s fair to say this change shouldn't be rolled into this patch, I've learned a bit on that front but these patches are little like being in a time machine :).. but with the melding of everything on larch I'm not sure its worth separating out a small detail like this with so much water under the bridge. But let me know if you think it must happen. [OK on the name change to NS_NET_STATUS_WAITING_FOR].

> > + return mProxyConnectStream != nsnull; > > Fixed from !!mSSLProxyConnectStream during merge. Please don't use !! for > pointer to bool conversion. > I left your change in place, but it violates the style guide which prohibits comparisons to nsnull. That's why it was written in that odd way to start with. > @@ +350,2 @@ > > if (trans->Caps() & NS_HTTP_ALLOW_PIPELINING) { > > pipeline->AddTransaction(trans); > > What if this fails? It might when pipeline is not a pipeline but a > transaction here. the pipeline variable is of type nsHttpPipeline - it cannot be simple transaction.

(In reply to comment #35) > Comment on attachment 537844 [details] [diff] [review] [review] > v9 (by Patrick McManus) merged > @@ +553,5 @@ > > + LOG(("nsHttpPipeline::WriteSegments [this=%x] Forced Delegation\n", > > + this)); > > + trans = Request(0); > > + } > > + > > Why this change? Why wasn't this needed before? Please explain and also > add a comment to the code. > This change is necessary to support the logic of setting up a CONNECT based tunnel through a proxy. m-c does not need this code because a transactions that does this does not support pipelining (see connection.cpp around line 400) - and on m-c that means the nsHttpPipeline datastructure is not used, but on the pipeline branch that structure is used in order to allow the connection to become a pipelining based connection after the tunnel is set up if other conditions allow. code will be commented too.

updated for most of review comments - still need to address some questions

> > @@ +627,5 @@ > > + // if the response has not started to arrive and the reason > > + // for failure is innocuous (e.g. not an SSL error) > > + if (!mResponseIsPartial && > > + (reason == NS_ERROR_NET_RESET || reason == NS_OK || > > + reason == NS_BASE_STREAM_CLOSED)) { > > Before this change we force restart whenever the response was complete. s/complete/empty > > After this change we force restart only when the response is complete and > the connection is closed with one of NS_ERROR_NET_RESET, NS_OK, > NS_BASE_STREAM_CLOSED errors. I think this code is correct. We only want to restart a closed pipeline element on those conditions which dicate the normal restart things (net_reset) plus a server closing a connection in between transactions (a pipeline specific hazard). If something like an SSL error happens and we restart anyway we just get caught in a loop (which does eventually exit due to a circuit breaker in the restart logic after some large number of iterations.. 10?). In both cases we restart with pipelining disabled (as part of the restart logic). on m-c that means nshttppipeline isn't used the second time so we only do one extra attempt. Of course, with these patches the nsHttpPipeline datastructure is used and so we see the more bizarre behavior.

> > ::: netwerk/protocol/http/nsHttpPipeline.cpp > > @@ +412,3 @@ > > nsAHttpTransaction *trans = Request(0); > > + if (!trans) > > + trans = Response(0); > > Why wasn't this needed before? > This is in getsecuritycallbacks(). This is going to be used when the transaction has moved from sending the request to reading the response. If this were called directly on the nsHttpTransaction no distinction would be drawn. Cases where something like GetInterface() obtains the callback information to look for a prompting interface would be one likely use case. As for "why not before", I did not document that sufficiently but am certain I ran across a case. > @@ +947,2 @@ > > > > + nsHttpPipeline *pipeline = new nsHttpPipeline(); > > I see a major issue here: you have removed check for > NS_HTTP_ALLOW_PIPELINING for the first transaction. If it is not allowed, > it is still added to a pipeline. You can admit that just adding a first > transaction to the pipeline is not actual pipelining. > > The problem is in restarting. IsReused is implemented a different way in > nsHttpPipeline then in nsHttpConnection. This is an interesting piece of feedback, thank you. It led me to make 2 changes in the patch I will update shortly: 1] introduce the mUtilizedPipeline flag. In cases where only 1 request has ever been outstanding (i.e. not really pipelining) it will be false and ::IsReused() will call through to get the base nsHttpConnection version of IsReused(). 2] I realized there is a scheduling problem with this version of the patch that goes away with the introduction of the Type and State patch. with the patch in 447866 a non pipelineable transaction (e.g. a POST) can be the first transaction on a connection (which is ok) but be followed in a pipeline by a non-pipelinable transaction (e.g. GET .gif). One of the advantages this patch has over m-c is that a connection started for use with a POST can support a GET pipeline at a later time, but that time really should be after the POST has completely finished the response. I don't think it is a spec violation, but its a bad thing from a head of line blocking point of view. (POST is much likelier to take a while and maybe close the connection). The type and state patch solves this inherently, so I hadn't thought about it from the pov of this patch.

feedback from comment 35, along with responses in comments 36 through 41

(In reply to comment #36) > > @@ +432,5 @@ > > > + case nsISocketTransport::STATUS_WAITING_FOR: > > Its clear to me the target of that event is Response(0), as the transaction > is moved to the response q after the request is sent (which is what the > event is about) - thus the change. > I don't agree. In the non-pipelining environment STATUS_WAITING_FOR is sent to the transaction (and is propagated up nevertheless request body) at the moment its request data has been fully sent out. This is IMO broken even with the current code on m-c. Please remove this change rather. I will file a new bug for the status events, CC you and comment on it. To keep this working as precise as possible is vital for channel and web timing API as well as for Firebug and some other monitoring extensions. Also for telemetry. > And yes, .... But let me know if you think it must happen. We can work in "parallel" on coding/reviews when we have more bugs and we are more independent if bugs are well separated. (In reply to comment #37) > > > + return mProxyConnectStream != nsnull; > I left your change in place, but it violates the style guide which prohibits > comparisons to nsnull. That's why it was written in that odd way to start > with. I see. OK, then it is up to you, both seems to be correct then. > > @@ +350,2 @@ > > > if (trans->Caps() & NS_HTTP_ALLOW_PIPELINING) { > > > pipeline->AddTransaction(trans); > > > > What if this fails? It might when pipeline is not a pipeline but a > > transaction here. > > the pipeline variable is of type nsHttpPipeline - it cannot be simple > transaction. Ha! Brain short circuit :) (In reply to comment #40) > > > > @@ +627,5 @@ > > > + // if the response has not started to arrive and the reason > > > + // for failure is innocuous (e.g. not an SSL error) > > > + if (!mResponseIsPartial && > > > + (reason == NS_ERROR_NET_RESET || reason == NS_OK || > > > + reason == NS_BASE_STREAM_CLOSED)) { > > The condition is: if the connection has been broken exactly after the complete response has been received and the reason was receive of RST or a graceful shutdown from the server side then explicitly reset the transaction (by closing it with NS_ERROR_NET_RESET). Otherwise, use the error state of the connection to close the transaction. The problem I saw here was that in the "otherwise" case the error code could also be NS_ERROR_NET_RESET or NS_OK that would lead to transaction restart, even the response had been received just partially. But now I realize there is !mReceivedData in the restart condition, so transaction (w/o a "restart-in-progress" logic) will never restart. So, you are right, the code is correct! (In reply to comment #41) > > > + if (!trans) > > > + trans = Response(0); > > > > Why wasn't this needed before? > > > > This is in getsecuritycallbacks(). Please just add a comment about the motivation. Let's go with this now. One day I would like to change the "security callbacks" stuff completely. This really should not go through the pipeline object.. > 2] I realized there is a scheduling problem with this version of the patch > that goes away with the introduction of the Type and State patch. with the Yes, I was thinking of this later as well. And the result is one of the reasons I wanted to merge the patches to a single one. Now we have to land them on m-c both together anyway to ensure stability. Could PipelineDepthAvailable() method, if !(Request(0)/Response(0)->Caps() & NS_HTTP_ALLOW_PIPELINING), return 0 and then always use that method before trying to AddTransaction on a pipeline where needed (probably already all places as I'm quickly looking into the new patch version, so no more drastic changes needed)?

(In reply to comment #43) > Could PipelineDepthAvailable() method, if !(Request(0)/Response(0)->Caps() & > NS_HTTP_ALLOW_PIPELINING), return 0 and then always use that method before > trying to AddTransaction on a pipeline where needed (probably already all > places as I'm quickly looking into the new patch version, so no more drastic > changes needed)? Ah! It is in the patch already! Great! :D

Comment on attachment 543242 [details] [diff] [review] v 11 Review of attachment 543242 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. Much smoother. r=honzab with the few comments addressed. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +4026,5 @@ > // on proxy errors, try to failover > if (mConnectionInfo->ProxyInfo() && > (mStatus == NS_ERROR_PROXY_CONNECTION_REFUSED || > mStatus == NS_ERROR_UNKNOWN_PROXY_HOST || > + mStatus == NS_ERROR_NET_RESET || /* could be aborted pipeline */ Sorry, this comment doesn't explain anything. As I see this is not a change needed for functionality of this particular patch. If change like this is needed for a stable pipelining infrastructure, then please create a new bug and give a detailed and deep explanation of why to add NET_RESET to conditions for using next configured proxy in line. This could also have security implications. I think detecting a proxy that blocks/breaks/aborts pipelining will more likely be detected with the pretest. ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +333,2 @@ > LOG(("nsHttpConnectionMgr::AddTransactionToPipeline [pipeline=%x]\n", pipeline)); > + Please don't add white spaces. @@ +335,3 @@ > NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread"); > + PRUint16 avail = pipeline->PipelineDepthAvailable(); > + As well here. @@ +680,5 @@ > + ent->mPendingQ.RemoveElementAt(pendingIndex); > + > + // adjust iterator to reflect coalesced queue > + pendingCount--; > + avail--; Please rather use everywhere where just increasing/decreasing a single variable the prefix form (--avail). It is in general more efficient. @@ +975,5 @@ > } > > + if (numAdded > 1) > + LOG((" pipelined %u transactions\n", numAdded)); > + White spaces added ::: netwerk/protocol/http/nsHttpPipeline.cpp @@ +304,5 @@ > PRBool > nsHttpPipeline::IsReused() > { > + if (!mUtilizedPipeline && mConnection) > + return mConnection->IsReused(); Thanks! @@ +453,5 @@ > nsAHttpTransaction *trans; > switch (status) { > case NS_NET_STATUS_RECEIVING_FROM: > + case NS_NET_STATUS_WAITING_FOR: > + // forward these only to the transaction currently receiving data As I commented earlier... @@ +589,5 @@ > nsresult rv; > > trans = Response(0); > + // This code deals with the establishment of a CONNECT tunnel through > + // an HTTP proxy. It is a precursor to the actual HTTP transaction. You promised a better comment here. @@ +673,5 @@ > + // if the response has not started to arrive and the reason > + // for failure is innocuous (e.g. not an SSL error) > + if (!mResponseIsPartial && > + (reason == NS_ERROR_NET_RESET || reason == NS_OK || > + reason == NS_BASE_STREAM_CLOSED)) { Please each "reason == NS_* ||" on a separate line.

ok. patch with feedback applied is reflected in larch. thanks. I think omitting net_status_waiting_for will come back to bite us but time will tell.

Integrating the bursty pipeline patch with the spdy code required one substantive change and a bunch of mechanical ones. For the mechanical ones I just updated the existing patch (I'll upload that in a minute). I broke out the more substantive bits into this separate patch so it can be reviewed more easily. It should be applied before the main bursty patch. The main issue at hand is that the nshttpconnection wraps the nsAHttpTransaction it has inside a spdy ojbect after it sees we are going to do spdy via npn. If that trans is really a pipeline ojbect then things go badly because methods such as RequestHeaders() can only work on a pipeline object if the transactions on it are processed in order - which spdy of course does not do. This isn't a problem on mozilla-central because without bursty the first transaction is never a pipeline object.. and after that the connection manager knows that spdy is being used so it doesn't pipeline either. The NPN stuff could be moved to connection manager, but that's not good for the base case which is just driven off a write of the http transaction request headers. The solution here is non-invasive. It creates a method for a nsAHttpTransaction to return a nsTArray of nsAHttpTransaction references representing the sub-transactions of a pipeline or spdysession. nsHttpConnection uses this to unpack the pipeline and repack it into a spdysession which would only be used on the first transaction[s] of a spdy connection. syntax note for reviewer: afaik "nsTArray<nsRefPtr<nsAHttpTransaction> >" is actually the preferred syntax as ">>" obviously also means something else. The OS X compiler wouldn't compile it.

Comment on attachment 581263 [details] [diff] [review] prepatch r0 v1 (takesubtransactions) Review of attachment 581263 [details] [diff] [review]: ----------------------------------------------------------------- More logical would be to first create the connection, check for NPN and only after that collecting the pending transactions and potentially also build the pipeline over it if not going SPDY. I didn't think of how much changes would that mean, but seems to me much cleaner then moving things from one object to another just because we've built the first object too soon, don't you think? We already put transactions to the pending queue and create transports for them first. After a transport is created with nsHalfOpenSocket, we create a connection and activate it with the first pending transaction. And also only after that we create a pipeline on it. In my opinion the NPN extension should be checked in nsHalfOpenSocket. Connection manager (or any better suited place) then should manage this discovery and use nsHttpConnection/SpdySession instead of nsHttpConnection/nsHttpPipeline. > The NPN stuff could be moved to connection manager, but that's not good for the base case which is just driven off a write of the http transaction request headers. I don't quit understand this argument. NPN is discovered during SSL negotiation and known sooner we send out any request data. Dropping r? until we come to a conclusion here. If we agree on the proposed solution, the comments bellow may be just ignored. ::: netwerk/protocol/http/SpdySession.cpp @@ +1842,5 @@ > + LOG3((" taking %d\n", mStreamTransactionHash.Count())); > + > + nsTArray<nsRefPtr<nsAHttpTransaction> > list; > + mStreamTransactionHash.Enumerate(TakeStream, &list); > + outTransactions = list; You may work with outTransactions directly. Indention. ::: netwerk/protocol/http/nsAHttpTransaction.h @@ +112,5 @@ > + // Returns NS_ERROR_ALREADY_OPENED if the subtransactions have been > + // at least partially written and cannot be moved. > + // > + virtual nsresult TakeSubTransactions( > + nsTArray<nsRefPtr<nsAHttpTransaction> > &outTransactions) = 0; Would be nicer to have a type for this: typedef nsTArray<nsRefPtr<nsAHttpTransaction> > AHttpTransactionArray; ::: netwerk/protocol/http/nsHttpConnection.cpp @@ +194,5 @@ > + // Wrap the old http transaction into the new spdy session > + // as the first stream. > + mSpdySession = new SpdySession(mTransaction, > + mSocketTransport, > + mPriority); You have to handle |rv| better here. - if rv == NS_ERROR_NOT_IMPLEMENTED, you can assume mTransaction contains nsHttpTransaction object - if rv == NS_ERROR_ALREADY_OPEN, you must not create the spdy session! not sure what to do in this case, probably abort the connection and log we screwed up. - otherwise, move the streams to spdy session Also, not precisely for review of this bug, but shouldn't you rather look for a preferred spdy connection instead of creating a new spdy session? Because if we connect a site, e.g. a CDN for images, the first time, we create 6 parallel spdy sessions, right? ::: netwerk/protocol/http/nsHttpPipeline.cpp @@ +378,5 @@ > + > + PRInt32 i, count = mRequestQ.Length(); > + for (i = 0; i < count; ++i) { > + nsAHttpTransaction *trans = Request(i); > + list.AppendElement(Request(i)); s/Request(i)/trans/ ? @@ +385,5 @@ > + mRequestQ.Clear(); > + > + LOG((" took %d\n", count)); > + > + outTransactions = list; Why don't you just directly add to the outTransactions? This means to allocate the array twice.

(In reply to Honza Bambas (:mayhemer) from comment #49) > Comment on attachment 581263 [details] [diff] [review] > prepatch r0 v1 (takesubtransactions) > > Review of attachment 581263 [details] [diff] [review]: > ----------------------------------------------------------------- > > More logical would be to first create the connection, check for NPN and only > after that collecting the pending transactions and potentially also build > the pipeline over it if not going SPDY. I didn't think of how much changes > would that mean, but seems to me much cleaner then moving things from one > object to another just because we've built the first object too soon, don't > you think? its a major change to existing functionality beacuse of the way SSL tunnels are done through http proxies (the plaintext CONNECT and response is exchanged before SSL is negotiated ... and that might contain proxy-auth that is part of the transaction) I don't think its worth it, at least at this time though it seems reasonable to pursue as a future todo item when all the various protocol bits are more settled. Shake one thing up at a time :) wrapping and unwrapping the pipeline for spdy is undesirable of course, but its actually a pretty uncommon event. SPDY is *very* sticky and persistent and this process never happens with a reused spdy session.. computationally I don't think it adds up to very much. it would only happen on the one (set of) transaction that forces then handshake and isn't sure if it is spdy or http/1. > Would be nicer to have a type for this: I rarely use synthetic types short of full classes - I find they obfuscate rather than abstract. > Because if we connect a site, e.g. a CDN for images, the first time, we > create 6 parallel spdy sessions, right? the case of just using 1 on a hostname that is not known of spdy/http already exists in the connection manager (around 1114 in connection mgr). the CI then notes whether the site was spdy or not to optimize that the nxt time around.

Comment on attachment 581266 [details] [diff] [review] v12 Review of attachment 581266 [details] [diff] [review]: ----------------------------------------------------------------- r-, see the last hunk comment. ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +1301,5 @@ > nsAHttpTransaction *trans = aTrans; > + > + /* use pipeline datastructure even if connection does not currently qualify, > + because it might later on (receipt of a Server header, for example) or > + change in request method for a subsequent transaction */ Check wording of this comment please. ::: netwerk/protocol/http/nsHttpPipeline.cpp @@ +477,5 @@ > NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread"); > > + // depending on timing this could be either the request or the response > + // that is needed - but they both go to the same host. A request for these > + // callbacks directlyin nsHttpTransaction would not make a distinction s/directlyin/directly in/ ::: netwerk/protocol/http/nsHttpTransaction.cpp @@ +1080,5 @@ > + // doesn't make any sense. Assume 0 bytes if there is no > + // other delimiter. > + LOG(("CONNECT response assumed to be 0 bytes.\n")); > + mContentLength = 0; > + } This belongs to a different bug. And also, BTW, doesn't fall under my r+ for the previous patch version you carry on...

> This belongs to a different bug. > I appreciate the catch - I don't know how that got in there.. I remember that code as a WIP approach to some completely different bug (eventually fixed another way iirc). Maybe I typed in the wrong window.. ?

(In reply to Patrick McManus [:mcmanus] from comment #50) > (In reply to Honza Bambas (:mayhemer) from comment #49) > > Comment on attachment 581263 [details] [diff] [review] > > prepatch r0 v1 (takesubtransactions) > > > > Review of attachment 581263 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > More logical would be to first create the connection, check for NPN and only > > after that collecting the pending transactions and potentially also build > > the pipeline over it if not going SPDY. I didn't think of how much changes > > would that mean, but seems to me much cleaner then moving things from one > > object to another just because we've built the first object too soon, don't > > you think? > > its a major change to existing functionality beacuse of the way SSL tunnels > are done through http proxies (the plaintext CONNECT and response is > exchanged before SSL is negotiated ... and that might contain proxy-auth > that is part of the transaction) With the current code, since you don't allow spdy when going though a proxy, in HalfOpenSocket you would simply not find ssl socket control. That is enough to not force on NPN negotiation and push the transport to upper layers. The CONNECT request should also be ripped out of nsHttpConnection out (also as part of work to support HTTPS proxies) to the HalfOpenSocket (or better some other class). That very much makes sense since we have a lot hacks (like ForceReadEvents() and some other flags) to process the connect request inside nsHttpConnection. Definitely not something that should block this bug. > the case of just using 1 on a hostname that is not known of spdy/http > already exists in the connection manager (around 1114 in connection mgr). > the CI then notes whether the site was spdy or not to optimize that the nxt > time around. Hmm.. it may be suboptimal.. Anyway, not for this bug.

(In reply to Honza Bambas (:mayhemer) from comment #53) > With the current code, since you don't allow spdy when going though a proxy, fwiw once we are confident in spdy I want to allow it through a proxy. It is just a simplifciation for the rollout to make data and bug reports easier to understand.

(In reply to Honza Bambas (:mayhemer) from comment #49) > - if rv == NS_ERROR_ALREADY_OPEN, you must not create the spdy session! not > sure what to do in this case, probably abort the connection and log we > screwed up. it would be a code logic screw up, not a data error - so I've just added an ns_abort specifically for that. Also to cover unexpected return values.

The only intended changes in here are for review comments from comment 51

Changes in this revision (in addition to those driven by review comments) 1] in StartSpdy() set mAllowPipelining = false. the busrty patch changes the default to allow pipelining to be true and it won't get set back to false again because of spdy until OnHeadersAvailable - so that leaves a window between when takesubstransactions() has been run and when we get to OHA. Turning it off in StartSpdy() fixes that. 2] in nshttppipeline::takesubtransactions it needs to consider the case where there are 0 transactions in the pipeline obj (I believe it has been canceled). The right path here is to just not try to unroll this - the nsAHttpTransaction works fine as is. That's what was being done in rev 1 of this patch, but it was really returning the wrong code.

Comment on attachment 586746 [details] [diff] [review] prepatch r0 v2 (takesubtransactions) Review of attachment 586746 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpConnection.cpp @@ +189,5 @@ > + nsresult rv = mTransaction->TakeSubTransactions(list); > + > + NS_ABORT_IF_FALSE(rv != NS_ERROR_ALREADY_OPENED, > + "TakeSubTranscations somehow called after " > + "nsAHttpTransaction began processing"); I want NS_ABORT_IF_TRUE here. Really a "mental gymnastic"... Translation: this fails when rv is NS_ERROR_ALREADY_OPENED. Ok. But I don't agree with just adding an assertion, sorry. When we change something and this will have a chance to fail and we won't catch it during review or automated tests, what will be the symptoms users will see and experience in optimized builds? How will we catch this? How will we identify bugs around this code screw-up? Will it crash? Will it load anyway? Will it be just a hidden bug? Will it hang? Feel free to add an assertion that will crash in debug builds to the if branch, though. @@ +194,5 @@ > + > + NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv) || > + rv == NS_ERROR_NOT_IMPLEMENTED, > + "unexpected result from " > + "nsAHttpTransaction::TakeSubTransactions()"); Translation: fails if NS_FAILED(rv) && rv != NS_ERROR_NOT_IMPLEMENTED. @@ +202,5 @@ > + > + // This is ok - treat mTransaction as a single real request. > + // Wrap the old http transaction into the new spdy session > + // as the first stream. > + mSpdySession = new SpdySession(mTransaction, According code in TakeSubTransactions() and also comment 57 ad 2, it may happen you add an empty pipeline as the first transaction. Also, in the production code, if it happens that mTransaction is SpdySession, then you will have SpdySession in SpdySession. @@ +222,5 @@ > + // AddStream() cannot fail > + mSpdySession->AddStream(list[index], mPriority); > + } > + } > + } This deserves some logs on what has happened. @@ +224,5 @@ > + } > + } > + } > + > + mSupportsPipelining = false; // dont use http/1 pipelines with spdy Good catch. ::: netwerk/protocol/http/nsHttpPipeline.cpp @@ +373,5 @@ > + // request queue could be empty if it was already canceled, in which > + // case it is safe to treat this as a cae without any > + // sub-transactions. > + if (!mRequestQ.Length()) > + return NS_ERROR_NOT_IMPLEMENTED; This will put this pipeline as the first transaction to the spdy session, right? Probably not something you want to do. Also, all transactions could just be sent out and there could be transactions is mResponseQ. NS_ERROR_NOT_IMPLEMENTED is then really not what you want to return. However, for me it is quit logical to just return an empty array and NS_OK. It's a fully valid case. If the pipeline is empty, we want to throw it simply away. This proposal also reminds me of one review comment on the spdy patch I later decided not to publish: I don't very much like to pass the first transaction via the constructor. Rather call AddStream explicitly would be IMO better, of course, if the current logic implementation allows it, but I expect the protocol does. @@ +376,5 @@ > + if (!mRequestQ.Length()) > + return NS_ERROR_NOT_IMPLEMENTED; > + > + if (mResponseQ.Length() || mRequestIsPartial) > + return NS_ERROR_ALREADY_OPENED; Maybe do the check in reverse order?

Comment on attachment 586745 [details] [diff] [review] v13 Review of attachment 586745 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab Thanks. If there is some major change in this patch after update of the prerequisites patch, please let me check. Otherwise free to land. ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +1300,5 @@ > nsHttpPipeline *pipeline = nsnull; > nsAHttpTransaction *trans = aTrans; > + > + /* Use pipeline datastructure even if connection does not currently qualify > + to pipeline this transaction because a different pipeline-elgible transaction s/elgible/eligible/ ?

Comment on attachment 586745 [details] [diff] [review] v13 Review of attachment 586745 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsAHttpConnection.h @@ +118,5 @@ > > + // Used to determine if the connection wants read events even though > + // it has not written out a transaction. Used when a connection has issued > + // a preamble such as a proxy ssl CONNECT sequence. > + virtual bool ForceReadEvents() = 0; Patrick, could you please rename this method to IsProxyConnectInProgress() ? You are using this flag in following patches on other places not to just to "force read events". It is very misleading name. Thanks.

Comment on attachment 586745 [details] [diff] [review] v13 Review of attachment 586745 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpPipeline.cpp @@ +784,5 @@ > trans = Request(i); > + if (mConnection && mConnection->ForceReadEvents()) > + trans->Close(reason); > + else > + trans->Close(NS_ERROR_NET_RESET); Ah, and I forgot about this one piece: Can you please explain why not to reset when processing CONNECT request? Consider pending r? until answered, please.

(In reply to Honza Bambas (:mayhemer) from comment #61) > ::: netwerk/protocol/http/nsHttpPipeline.cpp > @@ +784,5 @@ > > trans = Request(i); > > + if (mConnection && mConnection->ForceReadEvents()) > > + trans->Close(reason); > > + else > > + trans->Close(NS_ERROR_NET_RESET); > > Ah, and I forgot about this one piece: > > Can you please explain why not to reset when processing CONNECT request? > The proxy tunnel code is really awkward. We've got a failure on the CONNECT but the CONNECT is not one of the Request[] elements - it is a prerequisite to them. However the only notificiation mechanism we have for the connect is the code we pass to trans[0]->Close().. so we want to use whatever the real code is in that case, not override it with RESET. (It might actually be reset, or it might not). So that just leaves us with request[1..n], which is a tougher call. I feel that if they require a proxy and the proxy code is throwing an error that needs to be dealt with rather than just retrying the queue (without request[0]) which probably loops you back again to the same spot until the retry limit is reached. Again, if the underlying error is RESET will we still RESET them.

> > + NS_ABORT_IF_FALSE(rv != NS_ERROR_ALREADY_OPENED, > > + "TakeSubTranscations somehow called after " > > + "nsAHttpTransaction began processing"); > > I want NS_ABORT_IF_TRUE here. Really a "mental gymnastic"... > fwiw, as this code was removed based on another comment so it isn't worth much, I try and consistently use ABORT_IF_FALSE(precondition) because it is consistent with the assert() idiom. I find that more valuable than the naturalness of the precondition which always requires some inspection anyhow. > > @@ +202,5 @@ > > + > > + // This is ok - treat mTransaction as a single real request. > > + // Wrap the old http transaction into the new spdy session > > + // as the first stream. > > + mSpdySession = new SpdySession(mTransaction, > > According code in TakeSubTransactions() and also comment 57 ad 2, it may > happen you add an empty pipeline as the first transaction. it's true - but that's not a problem. > > Also, in the production code, if it happens that mTransaction is > SpdySession, then you will have SpdySession in SpdySession. SpdySession::TakeSubTransactions() does not return NOT_IMPLEMENTED - it unwraps any subtranscations. but program state prevents that from happening anyhow. > > ::: netwerk/protocol/http/nsHttpPipeline.cpp > @@ +373,5 @@ > > + // request queue could be empty if it was already canceled, in which > > + // case it is safe to treat this as a cae without any > > + // sub-transactions. > > + if (!mRequestQ.Length()) > > + return NS_ERROR_NOT_IMPLEMENTED; > > This will put this pipeline as the first transaction to the spdy session, > right? > > Probably not something you want to do. > It works ok - the nsA* abstraction of pipeline works ok as long as there is no multiplexing going on. And we have to convert something to be a spdysession becuase npn has negotiated spdy. > Also, all transactions could just be sent out and there could be > transactions is mResponseQ. NS_ERROR_NOT_IMPLEMENTED is then really not > what you want to return. > Nothing can be sent out yet at the state this is called in. I can move up the ALREADY_OPENED check to make the interface stronger as you suggest below.

Comment on attachment 596690 [details] [diff] [review] prepatch r0 v3 (takesubtransactions) Review of attachment 596690 [details] [diff] [review]: ----------------------------------------------------------------- Thanks r=honzab ::: netwerk/protocol/http/nsHttpConnection.cpp @@ +210,5 @@ > + NS_ABORT_IF_FALSE(false, > + "unexpected result from " > + "nsAHttpTransaction::TakeSubTransactions()"); > + mTransaction->Close(NS_ERROR_ABORT); > + return; Indention. @@ +221,5 @@ > + // Wrap the old http transaction into the new spdy session > + // as the first stream. > + mSpdySession = new SpdySession(mTransaction, > + mSocketTransport, > + mPriority); Please add a log that mTransaction has been moved to mSpdySesssion @@ +232,5 @@ > + for (PRInt32 index = 0; index < count; ++index) { > + if (!mSpdySession) { > + mSpdySession = new SpdySession(list[index], > + mSocketTransport, > + mPriority); Please add a log (different from the previous to recognize NS_FAILED(rv) from list.Length() == 1) about moving the transaction(s).

(In reply to Patrick McManus [:mcmanus] from comment #62) > the only notificiation mechanism we have for the connect is > the code we pass to trans[0]->Close().. Ah! Yes, that's it. Thanks. Can you please add a small comment about this? (In reply to Patrick McManus [:mcmanus] from comment #63) > > According code in TakeSubTransactions() and also comment 57 ad 2, it may > > happen you add an empty pipeline as the first transaction. > > it's true - but that's not a problem. Because of the abstraction? I can't say I like this but if you believe it works well then let's get it in. This code should one day be removed anyway, so it's a workaround. > but program state prevents that from happening anyhow. That's something that could get broken in the future... > It works ok - the nsA* abstraction of pipeline works ok as long as there is > no multiplexing going on. And we have to convert something to be a > spdysession becuase npn has negotiated spdy. You have a spdy session but I don't think you really need to have a transaction on it immediately. Can be a fresh spdy session for some time just inactive? (means w/o transactions (streams) on it ; in other words, how does the server behave when you open a session, but don't open any streams on it? Is there a timeout? Does the client must send something "immediately"?) > > Also, all transactions could just be sent out and there could be > > transactions is mResponseQ. NS_ERROR_NOT_IMPLEMENTED is then really not > > what you want to return. > > > > Nothing can be sent out yet at the state this is called in. I can move up > the ALREADY_OPENED check to make the interface stronger as you suggest below. OK. (Re NS_ABORT_IF_TRUE: I don't in any way want you to use it, I'm also looking at the condition as being put in ASSERT(), it just sometimes seems to be more readable to me inverted (and I'm not the only one).)

(In reply to Honza Bambas (:mayhemer) from comment #66) > > Because of the abstraction? yep. the abstraction only breaks down if you start multiplexing > 1 object. > Can be a fresh spdy session for some time just inactive? (means w/o > transactions (streams) on it ; in other words, how does the server behave > when you open a session, but don't open any streams on it? Is there a > timeout? Does the client must send something "immediately"?) The spec doesn't draw a distinction between a session that has seen streams and one that hasn't - in practice I've watched a connection sit there for over a minute without anything happening other than a SETTINGS frame coming down from the server. It was then used without a problem. SPDY incentives would seem to favor keeping the single connection alive whenever possible.

Comment on attachment 597460 [details] [diff] [review] take subtransactions pre patch v4 [checked-in] take subtranscations pre patch v4 attachment to inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/74126241b4aa

Sorry, I backed this out on inbound because something in the push appeared to cause a leak in mochitest-browser-chrome: https://hg.mozilla.org/integration/mozilla-inbound/rev/577da08878cb

The leak is perma orange and easy to trigger with an updated try build for any DEBUG mochi-other. That's good. This same patch was validated with try (no leak) based on a merge with friday's mozilla-central. It was applied and bounced out of inbound on tuesday. I am 95% sure that the checkin that triggers the issue is from monday: changeset: 89793:be7eba63e2ad user: Gavin Sharp <gavin@gavinsharp.com> date: Mon Mar 19 17:24:31 2012 -0700 summary: Bug 736878: also re-set default engine name when user opts in to search re-set, r=fryn I'm not sure why - the most likely candidate is that it adds a new test which triggers the behavior.

be7eba63e2ad added a test which actually connected to https://www.google.com (I've noted it in that bug) The transaction was cancelled (mochi page closed before transaction could be sent). This resulted in the leak due to pipeline::takesubtransactions returning NOT_IMPLEMENTED for the case where there are no transactions in the pipeline. The connection code treated the nsAHttpTransaction as if it were a plain transaction (instead of a pipeline object) and wrapped it directly in the SpdySession. This broke down because spdysession called setConnection on that object (which is not allowed to be done twice) and because there was no way for the pipeline object to signal completion because it had no transactions on it that would complete within the lifetime of the spdysession. I fixed this by having pipeline::takesubtransactions return NS_OK and an empty list when it is empty (instead of NOT_IMPLEMENTED). The NS_SUCCEEDED path in nshttpconnection now allows an empty list (it had previously aborted on that), and closes the connection when it sees one. Figuring out how to keep the connection alive in this very corner case was too ugly - maybe when we have the NullHttpTransaction merged we can plug that into this case as the place holder when the pipeline is empty. I also fixed 2 related uses of mUsingSpdy that were protecting deref of mSpdySession. mSpdySession should be checked against null - mUsingSpdy refers to NPN negotiation, it is possible (such as the error path I added above) to have done NPN=spdy/2 but not have a spdysession and shutdown code like IsAlive() might run into that.

This doesn't need to go to aurora because without bursty we never use the nsHttpPipeline data structure on the transaction that drives NPN.

I forgot to mention "Prepatch r1 [takesubtransactions follow on] v0" is just applied to the tip of m-c. (no deps)

Patrick, do you have reference to the try builds? I mean the repo again, I'd like to see the patch in the context and applied. Thanks.

(In reply to Patrick McManus [:mcmanus] from comment #74) > I fixed this by having pipeline::takesubtransactions return NS_OK and an > empty list when it is empty (instead of NOT_IMPLEMENTED). Comment 58 ? > The NS_SUCCEEDED > path in nshttpconnection now allows an empty list (it had previously aborted > on that), and closes the connection when it sees one. Figuring out how to > keep the connection alive in this very corner case was too ugly - maybe when > we have the NullHttpTransaction merged we can plug that into this case as > the place holder when the pipeline is empty. Or simply having an "empty" spdy session waiting for transactions?

Comment on attachment 608229 [details] [diff] [review] Prepatch r1 [takesubtransactions follow on] v0 Review of attachment 608229 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for that test! r=honzab Looks good.

(In reply to Honza Bambas (:mayhemer) from comment #77) > Patrick, do you have reference to the try builds? I mean the repo again, > I'd like to see the patch in the context and applied. Thanks. sounds like you found it - anyhow: https://tbpl.mozilla.org/?tree=Try&rev=11fb955f2cf3 (In reply to Honza Bambas (:mayhemer) from comment #78) > (In reply to Patrick McManus [:mcmanus] from comment #74) > > I fixed this by having pipeline::takesubtransactions return NS_OK and an > > empty list when it is empty (instead of NOT_IMPLEMENTED). > > Comment 58 ? > yeah, you were right there.. it actually does work ok other than the leak - that's the part I did't notice. (all real transactions run to completion, but the datastructure does not because of the cancel). > > The NS_SUCCEEDED > > path in nshttpconnection now allows an empty list (it had previously aborted > > on that), and closes the connection when it sees one. Figuring out how to > > keep the connection alive in this very corner case was too ugly - maybe when > > we have the NullHttpTransaction merged we can plug that into this case as > > the place holder when the pipeline is empty. > > Or simply having an "empty" spdy session waiting for transactions? that's worth looking at too. thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8656bbf0818 https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd62d76cc6d

https://hg.mozilla.org/mozilla-central/rev/f8656bbf0818 https://hg.mozilla.org/mozilla-central/rev/3dd62d76cc6d