607741 - NS_SOCKET_MAX_COUNT in nsSocketTransportService2.h is un-necessary limit

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

in bug 606719 we learned it is a very bad thing for the connection manager to 
exceed NS_SOCKET_MAX_COUNT (set at 50)

#define NS_SOCKET_MAX_COUNT    50
  //
    // the number of sockets that can be attached at any given time is
    // limited.  this is done because some operating systems (e.g., Win9x)
    // limit the number of sockets that can be created by an application.
    // AttachSocket will fail if the limit is exceeded.  consumers should
    // call CanAttachSocket and check the result before creating a socket.
    //
    PRBool CanAttachSocket() {
        return mActiveCount + mIdleCount < NS_SOCKET_MAX_COUNT;
    }


we should not have a limit like that.. just try and create the socket and deal with it when it fails.. any callback at the limit isn't really reliable if you are really bumping up against the OS limit of file descriptors because FD's are global to the process (not just the socket transport pool).

Basically all you can really do if you are truly out of fd's is to try again later which can certainly be done in a OS neutral way, but the good news is there are generally LOTS more than 50 fd's available and sometimes it certainly would be sensible to use them to scale up.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: raise socket limit to 550 in some environments v1

Raise limit SocketService limit from 50 to 550 in environments that support that: XP, Vista, Win 7, OS X, Linux.. Other legacy cases that cannot be easily queried for support are left at the old value. Win9x is confirmed to limit to 100 so leave this value at 50 for that case. On the unixy systems where fds and sockets are the same thing setrlimit leaving room for 250 non socket fds. (the os x default is 256 total fds including sockets, so this exceeds our historical wiggle room).

This patch changes the SocketService limit, but not the http max-connections default configuration. That remains at 30, but now can be configured higher than 50 if we wanted to and we probably do want to pending some telemetry - but that's another bug.

Even the socket list initial allocations remain at 50, and grow dynamically, so this should not have any impact on a default config and any use case that was working before.

Tested with max configs (1000 connections, 100 per server) on the 3 windows platforms plus osx 10.6 and linux to confirm behavior.

Comment 2

[Patrick McManus [:mcmanus]]

I forgot to mention - I went with a high limit afterall instead of a "try and then deal wtih failure" approach in the end because

1] the CanAttachSocket() idl would have to change

2] if we allow sockets to claim all the fd's up until failure it would probably mean cascading failures to things like sqlite openings etc..

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: raise socket limit to 550 in some environments v2

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: raise socket limit to 550 in some environments v3

fix bitrot

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 524547 [details] [diff] [review]
raise socket limit to 550 in some environments v3

ff5 candidate

Comment 6

[Jason Duell]

Comment on attachment 524547 [details] [diff] [review]
raise socket limit to 550 in some environments v3


> 
>+PRBool
>+nsSocketTransportService::GrowActiveList()
>+{
>+    if ((mActiveListSize + 100) > MaxCount())
>+        return PR_FALSE;
>+    
>+    mActiveListSize += 100;
>+    mActiveList = (SocketContext *)
>+        moz_xrealloc(mActiveList, sizeof(SocketContext) * mActiveListSize);
>+    mPollList = (PRPollDesc *)
>+        moz_xrealloc(mPollList, sizeof(PRPollDesc) * (mActiveListSize + 1));
>+    return PR_TRUE;
>+}
>+
>+PRBool
>+nsSocketTransportService::GrowIdleList()
>+{
>+    if ((mIdleListSize + 100) > MaxCount())
>+        return PR_FALSE;
>+    
>+    mIdleListSize += 100;
>+    mIdleList = (SocketContext *)
>+        moz_xrealloc(mIdleList, sizeof(SocketContext) * mIdleListSize);
>+    return PR_TRUE;
>+}

Make these two functions grow up to MaxCount.  Right now they could grow up to Maxcount - 99 if they fail the initial if test.

+r with that

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: raise socket limit to 550 in some environments v4

updates from comment 6.. r=jduell

Comment 8

[Patrick McManus [:mcmanus]]

pushed to cedar
http://hg.mozilla.org/projects/cedar/rev/649e429d342f

Comment 9

[Benjamin Stover (:stechz)]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/649e429d342f

Comment 10

[(no longer active)]

Backed out because of mobile orange:

http://hg.mozilla.org/mozilla-central/rev/f8c2ffe03a51

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: : raise socket limit to 550 in some environments v5

fix mobile orange by considering case where sockettransport is not up during confiruation of nshttphandler

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 525230 [details] [diff] [review]
: raise socket limit to 550 in some environments v5

I tried this patch on a local Fennec Maemo build and it worked fine. I made a deb, installed it and launched it.

Previously, installing the deb would fail because we have a post install step that silently launches Fennec.

I was never able to reproduce the Android failures, but those cleared up after the backout as well.

Overall, I have some confidence that this patch won't break Fennec. The cedar branch should be running mobile tests now, if you want to land it there first.

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: raise socket limit to 550 in some environments v6

This patch is a simpler way to fix the mobile problem, using a static routine to do the OS FD limit exploration rather than the coordination approach in v5.

Jason, if you r+ and push this tonight (which I hope you will), please also re-push 648570 which depends on this patch. (no code changes are needed for 648570).

I have sent it to try just to be thorough: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d7bd3a0cbb36

Comment 14

[Jason Duell]

Attachment: raise socket limit to 550 in some environments v7

v6 had a race condition:  you set mMaxCount, then gSocketTransportService, and then assumed that another thread could safely read mMaxCount if gSocketTransportService was set (but in the absence of a lock there's no guarantee that the writes will have occurred in order).   Unlikely to race in practice, but I've fixed it by splitting out maxCount into a global static variable, and use PR_CallOnce to ensure it's only initialized once.

Comment 15

[Jason Duell]

http://hg.mozilla.org/mozilla-central/rev/e6d044d30abf

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out for not compiling on any platform.
http://hg.mozilla.org/mozilla-central/rev/171d203d16b1

Comment 17

[Jason Duell]

Attachment: raise socket limit to 550 in some environments v8

Sigh.  Was missing some stuff needed to link correctly.

Comment 18

[Patrick McManus [:mcmanus]]

(In reply to comment #14)
> Created attachment 525309 [details] [diff] [review]
> raise socket limit to 550 in some environments v7
> 
> v6 had a race condition:  you set mMaxCount, then gSocketTransportService, and
> then assumed that another thread could safely read mMaxCount if
> gSocketTransportService was set (but in the absence of a lock there's no
> guarantee that the writes will have occurred in order). 

Personally I'm not convinced that reordering across multiple functions is a problem, but I have no objection to v8. I assume it is undergoing some testing and you will push it when it clears that?

Comment 19

[Patrick McManus [:mcmanus]]

http://hg.mozilla.org/mozilla-central/rev/f48bde5681cc 
(v8)