Last Comment Bug 607741 - NS_SOCKET_MAX_COUNT in nsSocketTransportService2.h is un-necessary limit
: NS_SOCKET_MAX_COUNT in nsSocketTransportService2.h is un-necessary limit
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: x86_64 Linux
: -- normal with 2 votes (vote)
: ---
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 634278 648570
  Show dependency treegraph
 
Reported: 2010-10-27 13:26 PDT by Patrick McManus [:mcmanus]
Modified: 2012-03-29 02:10 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
raise socket limit to 550 in some environments v1 (18.37 KB, patch)
2011-01-16 07:09 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
raise socket limit to 550 in some environments v2 (18.80 KB, patch)
2011-01-16 07:34 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
raise socket limit to 550 in some environments v3 (18.80 KB, patch)
2011-04-07 17:20 PDT, Patrick McManus [:mcmanus]
jduell.mcbugs: review+
Details | Diff | Splinter Review
raise socket limit to 550 in some environments v4 (18.94 KB, patch)
2011-04-07 23:38 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
: raise socket limit to 550 in some environments v5 (22.21 KB, patch)
2011-04-11 17:25 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
raise socket limit to 550 in some environments v6 (19.25 KB, patch)
2011-04-11 19:19 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
raise socket limit to 550 in some environments v7 (19.08 KB, patch)
2011-04-11 22:35 PDT, Jason Duell [:jduell] (needinfo me)
jduell.mcbugs: review+
Details | Diff | Splinter Review
raise socket limit to 550 in some environments v8 (19.65 KB, patch)
2011-04-12 01:06 PDT, Jason Duell [:jduell] (needinfo me)
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2010-10-27 13:26:23 PDT
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] 2011-01-16 07:09:19 PST
Created attachment 504246 [details] [diff] [review]
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] 2011-01-16 07:12:13 PST
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] 2011-01-16 07:34:23 PST
Created attachment 504250 [details] [diff] [review]
raise socket limit to 550 in some environments v2
Comment 4 Patrick McManus [:mcmanus] 2011-04-07 17:20:47 PDT
Created attachment 524547 [details] [diff] [review]
raise socket limit to 550 in some environments v3

fix bitrot
Comment 5 Patrick McManus [:mcmanus] 2011-04-07 18:09:24 PDT
Comment on attachment 524547 [details] [diff] [review]
raise socket limit to 550 in some environments v3

ff5 candidate
Comment 6 Jason Duell [:jduell] (needinfo me) 2011-04-07 20:19:31 PDT
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] 2011-04-07 23:38:58 PDT
Created attachment 524585 [details] [diff] [review]
raise socket limit to 550 in some environments v4

updates from comment 6.. r=jduell
Comment 8 Patrick McManus [:mcmanus] 2011-04-08 11:43:18 PDT
pushed to cedar
http://hg.mozilla.org/projects/cedar/rev/649e429d342f
Comment 9 Benjamin Stover (:stechz) 2011-04-09 13:05:09 PDT
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/649e429d342f
Comment 10 :Ehsan Akhgari 2011-04-11 10:33:54 PDT
Backed out because of mobile orange:

http://hg.mozilla.org/mozilla-central/rev/f8c2ffe03a51
Comment 11 Patrick McManus [:mcmanus] 2011-04-11 17:25:52 PDT
Created attachment 525230 [details] [diff] [review]
: 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?) 2011-04-11 18:33:45 PDT
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] 2011-04-11 19:19:51 PDT
Created attachment 525274 [details] [diff] [review]
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 [:jduell] (needinfo me) 2011-04-11 22:35:50 PDT
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).   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 [:jduell] (needinfo me) 2011-04-11 22:43:07 PDT
http://hg.mozilla.org/mozilla-central/rev/e6d044d30abf
Comment 16 David Baron :dbaron: ⌚️UTC-8 2011-04-12 00:02:18 PDT
Backed out for not compiling on any platform.
http://hg.mozilla.org/mozilla-central/rev/171d203d16b1
Comment 17 Jason Duell [:jduell] (needinfo me) 2011-04-12 01:06:44 PDT
Created attachment 525329 [details] [diff] [review]
raise socket limit to 550 in some environments v8

Sigh.  Was missing some stuff needed to link correctly.
Comment 18 Patrick McManus [:mcmanus] 2011-04-12 06:44:59 PDT
(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] 2011-04-13 06:18:24 PDT
http://hg.mozilla.org/mozilla-central/rev/f48bde5681cc 
(v8)

Note You need to log in before you can comment on or make changes to this bug.