Note: There are a few cases of duplicates in user autocompletion which are being worked on.

NS_SOCKET_MAX_COUNT in nsSocketTransportService2.h is un-necessary limit

RESOLVED FIXED

Status

()

Core
Networking: HTTP
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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.
Attachment #504246 - Flags: review?(honzab.moz)
(Assignee)

Comment 2

7 years ago
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..
(Assignee)

Comment 3

7 years ago
Created attachment 504250 [details] [diff] [review]
raise socket limit to 550 in some environments v2
Attachment #504246 - Attachment is obsolete: true
Attachment #504250 - Flags: review?(honzab.moz)
Attachment #504246 - Flags: review?(honzab.moz)
(Assignee)

Updated

7 years ago
Blocks: 634278
(Assignee)

Comment 4

6 years ago
Created attachment 524547 [details] [diff] [review]
raise socket limit to 550 in some environments v3

fix bitrot
Attachment #524547 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #504250 - Flags: review?(honzab.moz)
(Assignee)

Comment 5

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

ff5 candidate
Attachment #524547 - Flags: review? → review?(jduell.mcbugs)
Attachment #504250 - Attachment is obsolete: true
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
Attachment #524547 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 524585 [details] [diff] [review]
raise socket limit to 550 in some environments v4

updates from comment 6.. r=jduell
Attachment #524547 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 648570
(Assignee)

Comment 8

6 years ago
pushed to cedar
http://hg.mozilla.org/projects/cedar/rev/649e429d342f
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/649e429d342f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Backed out because of mobile orange:

http://hg.mozilla.org/mozilla-central/rev/f8c2ffe03a51
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

6 years ago
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
Attachment #524585 - Attachment is obsolete: true
Attachment #525230 - Flags: review?(jduell.mcbugs)
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.
(Assignee)

Comment 13

6 years ago
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
Attachment #525230 - Attachment is obsolete: true
Attachment #525274 - Flags: review?(jduell.mcbugs)
Attachment #525230 - Flags: review?(jduell.mcbugs)
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.
Attachment #525274 - Attachment is obsolete: true
Attachment #525309 - Flags: review+
Attachment #525274 - Flags: review?(jduell.mcbugs)
http://hg.mozilla.org/mozilla-central/rev/e6d044d30abf
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Backed out for not compiling on any platform.
http://hg.mozilla.org/mozilla-central/rev/171d203d16b1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 525329 [details] [diff] [review]
raise socket limit to 550 in some environments v8

Sigh.  Was missing some stuff needed to link correctly.
Attachment #525309 - Attachment is obsolete: true
Attachment #525329 - Flags: review+
(Assignee)

Comment 18

6 years ago
(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?
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/mozilla-central/rev/f48bde5681cc 
(v8)
(Assignee)

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.