Closed
Bug 1104993
Opened 9 years ago
Closed 9 years ago
fix http connection manager half open accounting
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
Attachments
(1 file)
3.83 KB,
patch
|
u408661
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Attachment #8528589 -
Flags: review?(hurley)
Assignee | ||
Updated•9 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•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5c3f68c5866c
Assignee | ||
Comment 4•9 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•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/216d82ba7542
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/216d82ba7542
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•