1104993 - fix http connection manager half open accounting

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

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.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: fix connection manager half open accounting

Comment 2

[u408661]

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.

Comment 3

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=5c3f68c5866c

Comment 4

[Patrick McManus [:mcmanus]]

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.

Comment 5

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/216d82ba7542

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/216d82ba7542