Closed Bug 951293 Opened 6 years ago Closed 6 years ago

Fix limiting of speculative connections

Categories

(Core :: Networking, defect, minor)

x86_64
Windows 7
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: u408661, Assigned: u408661)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch busted_original.patch (obsolete) — Splinter Review
Over in bug 948757, it was noted that a page with a lot of links could end up flooding a server with connections because of predict on link hover behavior from the seer. That bug has been fixed, but there is an underlying issue of improper limiting of any speculative connections that also needs fixed. The original patch also fixed the underlying issue, but caused mochitest-bc timeouts, so we decided to land a band-aid while filing a follow-up to fix the issues with the fix to the underlying issue. This is that bug.
Attached patch patch (obsolete) — Splinter Review
And of course now I can't reproduce the mochitest-bc timeouts from the original patch - https://tbpl.mozilla.org/?tree=Try&rev=61c1c3fa4d68 (and previous failed try runs and m-i pushes have been removed from tbpl, so I can't try to debug from there).

IIRC, the timeouts were in some media-related tests, and Honza has said those are very unreliable anyway. That, plus my current lack of reproductions makes me want to say let's try this one again. If it fails on m-i, at least then I'll have a data point to look at to try to fix the issue.
Attachment #8348908 - Attachment is obsolete: true
Attachment #8364618 - Flags: review?(mcmanus)
Comment on attachment 8364618 [details] [diff] [review]
patch

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

sorry about the delay. too many planes and meetings - not enough code time.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2596,4 @@
>      }
>  
>      if (mNumHalfOpenConns < parallelSpeculativeConnectLimit &&
> +        ent->mIdleConns.Length() < parallelSpeculativeConnectLimit &&

I don't get it :(

parallelSpeculativeConnectLimit seems to always be 6 - either via the default pref value from httphandler when seer is not in use, or overloaded to 6 when the seer is in use.

so, assuming no seer, this change says Create a transport if there are less than 6 connections for the new transaction.. but if, again in the no-seer case, there is 1 idle connection then we don't want to be creating lots of connections - we just want to go ahead and use the one we've already got.

would it work to change this clause to

(ignoreidle && (ent->mIdleConns.Length() < parallelSpeculativeConnectLimit)) ||
  !ent->mIdleConns.Length()

?
Attachment #8364618 - Flags: review?(mcmanus)
Ah, indeed, that would make much more sense :) Glad it bounced the last time, since we both missed that! I'll fix it up and throw a new patch against the wall to see if it sticks.
Attached patch patch v2Splinter Review
Here's one, exact same clause you suggested. Running through try, let's see what breaks: https://tbpl.mozilla.org/?tree=Try&rev=0824fb33138a (now in the right bug!)
Attachment #8364618 - Attachment is obsolete: true
Attachment #8366895 - Flags: review?(mcmanus)
Comment on attachment 8366895 [details] [diff] [review]
patch v2

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

r=mcmanus assuming that try run finishes green
Attachment #8366895 - Flags: review?(mcmanus) → review+
Try is green, will land once everything opens again.
https://hg.mozilla.org/mozilla-central/rev/d7cf9038a7ec
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.