775515 - nsHttpConnectionMgr::RestrictConnections should not restrict on 1/2 complete half open

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

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]]

Attachment: patch 0

Comment 2

[Honza Bambas (:mayhemer)]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18671e974655

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18671e974655