Last Comment Bug 775515 - nsHttpConnectionMgr::RestrictConnections should not restrict on 1/2 complete half open
: nsHttpConnectionMgr::RestrictConnections should not restrict on 1/2 complete ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 16 Branch
: x86_64 Linux
: -- enhancement (vote)
: mozilla17
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 762162
  Show dependency treegraph
 
Reported: 2012-07-19 06:43 PDT by Patrick McManus [:mcmanus]
Modified: 2012-07-20 21:06 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0 (5.87 KB, patch)
2012-07-19 06:46 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2012-07-19 06:43:10 PDT
Over in bug 762162 I noted that a half-open connection that had already completed its primary (but had an outstanding hung backup connection) was causing the restrictconnection() logic to kick in.

That wasn't the intent there.. the nature of the syn-retry logic is that one of the connections might linger on for a long time due to loss, thus the parallel backup attempt.

that isn't the cause of bug 762162, but counting never-connected half opens is a better policy here.

The code change here looks bigger than it is because
 1] there was some code that already counted unconnected half opens in another function - so I broke that out and put it in its own routine
 2] ~nsHalfOpen was calling Restrictconnections() in case the dtor was lifting a restriction so it could call processPendingQ.. since restrictConnection() now deref's the halfopen (a bad thing to do from a dtor), it now calls processpending a little bit more liberally. I don't think that's a big deal.
Comment 1 Patrick McManus [:mcmanus] 2012-07-19 06:46:56 PDT
Created attachment 643825 [details] [diff] [review]
patch 0
Comment 2 Honza Bambas (:mayhemer) 2012-07-19 14:51:22 PDT
Comment on attachment 643825 [details] [diff] [review]
patch 0

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

This is a good change, hope we don't break anything..

r=honzab

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2345,1 @@
>              gHttpHandler->ConnMgr()->ProcessPendingQForEntry(mEnt);

As I see this now, I'm a bit worried that when somebody changes something, we could cause leak or hang for unpredictable time that will prevent dtor to call.  Maybe (in another bug) we could move this to some upper level method, e.g. where the half open is removed from the array.

@@ +3045,5 @@
>      return mGreenDepth;
>  }
> +
> +PRUint32
> +nsHttpConnectionMgr::nsConnectionEntry::UnconnectedHalfOpens()

Since you also use pendingHalfOpens variable, I'd suggest also to change name of this method to PendingHalfOpens().  Up to you.
Comment 3 Patrick McManus [:mcmanus] 2012-07-20 06:46:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/18671e974655
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-20 21:06:31 PDT
https://hg.mozilla.org/mozilla-central/rev/18671e974655

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