1355700 - Count the available connections correctly at nsHttpConnectionMgr::AtActiveConnectionLimit

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kershaw Chang [:kershaw]]

This is a regression of bug 1312782.

Since nsHttpConnectionMgr::AvailableNewConnectionCount [1] always returns an unsigned integer, we handled the available count for urgent-start case incorrectly.


[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/netwerk/protocol/http/nsHttpConnectionMgr.cpp#946-948

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: Correct the count of available new connection

Summary:
 - Change the return value of AvailableNewConnectionCount to int32_t
 - Fix the regression of counting connections for urgent-start transactions

Thanks.

Comment 2

[Honza Bambas (:mayhemer)]

Dragana, just FYI when you are looking at bug 1355539 in case this patch happens to be related.  But from quickly looking at it, it's probably not...

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8857330 [details] [diff] [review]
Correct the count of available new connection

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1030,5 @@
>      LOG(("nsHttpConnectionMgr::AvailableNewConnectionCount "
>           "total connection count = %d, limit %d\n",
>           totalCount, maxPersistConns));
>  
> +    return maxPersistConns - totalCount;

you are mixing uint16 and uint32 and the result is int32.  that's not good.  you need to static cast or something.

@@ +1184,4 @@
>      if (caps & NS_HTTP_URGENT_START) {
> +        if (availableConnections < 0 &&
> +            (static_cast<int32_t>(Abs(availableConnections)) >=
> +            static_cast<int32_t>(mMaxUrgentExcessiveConns))) {

This could be better written as:

// AvailableNewConnectionCount may return a negative number when we are over
// the default per-host limit.
if (-availableConnections > static_cast<int32_t>(mMaxUrgentExcessiveConns)) {
}

It took a while to decypher this, since IMO it's naturally expected that AvailableNewConnectionCount returns an unsigned result.  

We could go with this 'signed' approach is we find a good name for the AvailableNewConnectionCount method.  This name simply doesn't reflect the return value.

ConnectionAvailability?  ConnectionBalance?  I really don't know, what may show that it's not a good idea to let it return a signed int.  You can always somewhat return to the code as it was before https://bugzilla.mozilla.org/attachment.cgi?id=8851541&action=diff#a/netwerk/protocol/http/nsHttpConnectionMgr.cpp_sec7

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +474,5 @@
>                                     bool considerAll);
>  
>      // Given the connection entry, return the available count for creating
>      // new connections. Return 0 if the active connection count is
>      // exceeded |mMaxPersistConnsPerProxy| or |mMaxPersistConnsPerHost|.

and the comment definitely needs an update too

Comment 4

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Comment on attachment 8857330 [details] [diff] [review]
> Correct the count of available new connection
> 
> Review of attachment 8857330 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +1030,5 @@
> >      LOG(("nsHttpConnectionMgr::AvailableNewConnectionCount "
> >           "total connection count = %d, limit %d\n",
> >           totalCount, maxPersistConns));
> >  
> > +    return maxPersistConns - totalCount;
> 
> you are mixing uint16 and uint32 and the result is int32.  that's not good. 
> you need to static cast or something.
> 

Got it. Thanks.

> @@ +1184,4 @@
> >      if (caps & NS_HTTP_URGENT_START) {
> > +        if (availableConnections < 0 &&
> > +            (static_cast<int32_t>(Abs(availableConnections)) >=
> > +            static_cast<int32_t>(mMaxUrgentExcessiveConns))) {
> 
> This could be better written as:
> 
> // AvailableNewConnectionCount may return a negative number when we are over
> // the default per-host limit.
> if (-availableConnections > static_cast<int32_t>(mMaxUrgentExcessiveConns)) {
> }
> 
> It took a while to decypher this, since IMO it's naturally expected that
> AvailableNewConnectionCount returns an unsigned result.  
> 

Agree. It's too complicated and also hard to read.

> We could go with this 'signed' approach is we find a good name for the
> AvailableNewConnectionCount method.  This name simply doesn't reflect the
> return value.
> 
> ConnectionAvailability?  ConnectionBalance?  I really don't know, what may
> show that it's not a good idea to let it return a signed int.  You can
> always somewhat return to the code as it was before
> https://bugzilla.mozilla.org/attachment.cgi?id=8851541&action=diff#a/netwerk/
> protocol/http/nsHttpConnectionMgr.cpp_sec7
> 

Maybe we should split |AvailableNewConnectionCount| into two functions:
TotalActiveConnections and MaxPersistConnections.

This should make the code easier to read.
Moreover, I can change the code as it was before, which is clear and less error-prone.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Correct the count of available new connection - v2

Summary:
 - Split AvailableNewConnectionCount into two functions
 - Change the code in |AtActiveConnectionLimit| back

Thanks.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8858769 [details] [diff] [review]
Correct the count of available new connection - v2

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

lgtm, thanks

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1097,3 @@
>  
>      if (caps & NS_HTTP_URGENT_START) {
> +        if (totalCount >= static_cast<uint32_t>(mMaxUrgentExcessiveConns + maxPersistConns)) {

do we still need the static_cast?

Comment 7

[Honza Bambas (:mayhemer)]

and please push to try before landing and also make sure the problem that you found this issue through is fixed.

Comment 8

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b7c770c4dd4d9ba40caefe7ce8034a0fed0b3f

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Correct the count of available new connection, r=honzab

Carry reviewer's name.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2fccaa1a87a
Correct the count of available new connections. r=honzab

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2fccaa1a87a