fix http connection manager half open accounting

RESOLVED FIXED in mozilla36

Status

()

Core
Networking: HTTP
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

32 Branch
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
one path in connection manager removed a half open socket from the connection list twice - once explicitly and once via abandon.. fixed that to just call abandon.

The effect of that would just have been to suppress speculative connections under some conditions. Pretty minor.

also added some asserts around the accounting of that counter.
(Assignee)

Comment 1

3 years ago
Created attachment 8528589 [details] [diff] [review]
fix connection manager half open accounting
Attachment #8528589 - Flags: review?(hurley)
(Assignee)

Updated

3 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment on attachment 8528589 [details] [diff] [review]
fix connection manager half open accounting

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

r=me with a satisfactory resolution to the comment below (either explaining why I'm wrong, or making the change I mention)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3901,5 @@
> +    // A failure to create the transport object at all
> +    // will result in it not being present in the halfopen table. That's expected.
> +    if (mHalfOpens.RemoveElement(halfOpen)) {
> +
> +        if (halfOpen->IsSpeculative()) {

I'm not sure we want to move this inside the if... even if we don't create the transport, that still counts as an unused speculative connection (because we couldn't use it!), and so it should be counted as such.
Attachment #8528589 - Flags: review?(hurley) → review+
(Assignee)

Comment 3

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=5c3f68c5866c
(Assignee)

Comment 4

3 years ago
wrt comment 2

that code is trying to count the number of times we mispredicted... moving it to the conditional means it will be skipped in two circumstances

1] when the socketTransport couldn't actually be created (this happens in one of the xpcshell tests)... this isn't really a misprediction as much as a resource issue. You could argue this one either way because neither is really correct. But its totally in the noise and I like the new way a bit more.

2] when entry::removehalfopen() is called for the same half more than once. This shouldn't be happening anymore with this patch, but with this change its harmless to do so.
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/216d82ba7542
https://hg.mozilla.org/mozilla-central/rev/216d82ba7542
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.