necko ignores settings for max-persistent-connections-per-server

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
Networking: HTTP
--
minor
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Stephan, Assigned: Darin Fisher)

Tracking

({fixed-aviary1.0, fixed1.7.5, regression})

Other Branch
mozilla1.8beta1
x86
Windows XP
fixed-aviary1.0, fixed1.7.5, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7) Gecko/20040707 Firefox/0.9.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7) Gecko/20040707 Firefox/0.9.2

A few days ago, I updated to Fx 0.9.2 after uninstalling 0.8.

While everything was fine with 0.8, Fx 0.9 seems to ignore the individual
settings for the # of max. concurrent downloads. No matter what number I put in
my user.js for 'network.http.max-persistent-connections-per-server', Fx does not
allow more than two concurrent downloads from the same server.

When I type 'about:config', the settings are shown correctly (e.g. an integer
value of 8 for the above mentioned), but Fx simply ignores them.



Reproducible: Always
Steps to Reproduce:
1. Install Fx 0.9.2
2. Go to www.mozilla.org/products/mozilla1.x/ for example.
3. Try to download all three different versions of Mozilla at the same time.

Actual Results:  
The third download does not start at once, but only after the first one is
finished or one of the first two is aborted. It is impossible to download more
than two files from the same server concurrently.

Expected Results:  
Start the third download immediately due to the settings in user.js & prefs.js:

user_pref("network.http.max-persistent-connections-per-server", 8);

The bug occurs independently of the theme in use and the installed extensions.
(Reporter)

Comment 1

14 years ago
Important note:

The problem is NOT caused by a server-side download restriction! A lot of people
(including myself) have noticed that (see MozillaZine Forum for example), even
on websites where Fx 0.8 or other browsers can download more than two files
simultaneously.

Comment 2

14 years ago
This WFM with the Firefox 0.9.1 release build (win2000).
Keywords: regression
(Assignee)

Comment 3

14 years ago
please provide a mozilla HTTP log per instructions here:

http://www.mozilla.org/projects/netlib/http/http-debugging.html

thanks!

Updated

14 years ago
Assignee: bugs → darin
Severity: major → normal
Component: Download Manager → Networking: HTTP
Product: Firefox → Browser
QA Contact: bmo → core.networking.http
Version: unspecified → Other Branch

Comment 4

14 years ago
Created attachment 153949 [details]
http log

http log

set http://uk.altn.com/AntiVirus/Release/ to be home page
started Firefox
clicked /AntiVirus/Release/av224_en.exe, download started
clicked /AntiVirus/Release/av224_fr.exe, download started
clicked /AntiVirus/Release/av224_ge.exe, nothing happened
canceled one of the downloads in progress, dialog for third download appeared
and download commenced when OK'd

Comment 5

14 years ago
log was with a current aviary branch installer build on win2k.

I've just gone back a bit - as I said, it worked for me using 0.9.1 release
build, but using the aviary branch build from 20040703, it doesn't seem to work.

Comment 6

14 years ago
Just FYI...   It seems that max-persistent-connections-per-server is locked at 2
and max-connections-per-server is locked at 8.   You can toggle between the 2
behaviors by toggling keep-alive.  I can provide supporting http traces as well
as an Ethereal trace if it'll help further.
(Reporter)

Comment 7

14 years ago
Here's some more information, as some users in the German firefox forum found out:

By changing the values for max. connections and/or pipelining manually in the
file all.js in the "Mozilla Firefox/greprefs" directory, Fx accepts those
settings after the next restart and behaves like it should. Thus it seems that
this is the only file which values are accepted.

Changes made to the user.js, prefs.js or directly under about:config are shown
correctly, but they do not have any effect.
(Assignee)

Comment 8

14 years ago
Line 1552 of nsHttpHandler.cpp has this comment:

  // XXX should probably shutdown and init the conn mgr.

So, it would seem that observing pref changes, which apply to connection
management, is not implemented.  If you hand edit the same pref in the all.js
that ships with Firefox (inside the greprefs directory), then the pref would be
honored.

The solution to this bug is to shutdown and re-init the http connection manager.
 That should be a trivial patch.
Severity: normal → minor
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
(Assignee)

Comment 9

14 years ago
Created attachment 154664 [details] [diff] [review]
v1 patch

This should work.  I haven't rigorously tested it yet, but it should do the
trick.	If you have the ability to compile Mozilla for yourself, then please
give this patch a try.	Thanks!
(Assignee)

Updated

14 years ago
Attachment #154664 - Flags: review?(cbiesinger)
(Assignee)

Updated

14 years ago
Summary: Fx ignores settings for max-persistent-connections-per-server → necko ignores settings for max-persistent-connections-per-server
(Assignee)

Comment 10

14 years ago
OK, I suspect this bug may have been triggered by subtle changes in the timing
between profile selection and HTTP handler initialization.  If it ever happened
in the past that the profile was selected and prefs.js was parsed prior to the
HTTP handler being initialized, then this bug would not be seen.  (The exception
is when modifying the pref via about:config ... it would not take affect until
after a restart.)  It seems that today in firefox, the HTTP handler is
initialized before prefs.js has been parsed from the profile directory.  It's
unclear what is causing this.  I'm not sure it is important to know why this was
caused, since this bug exists when using about:config to change the pref during
a browser session.

Note: even if we have a lot of HTTP prefs in prefs.js, restarting the connection
manager should be relatively inexpensive.  Initialization is very cheap,
amounting to little more than a call to do_GetService, and shutdown is likewise
cheap when there are no connections or pending transactions to abort.
(Assignee)

Updated

14 years ago
Whiteboard: [patch]
Attachment #154664 - Flags: review?(cbiesinger) → review+

Comment 11

14 years ago
The ordering problem must have been fixed since FF 0.9.2 was released because
the AVIARY nightlies will obey the settings if the brower is restarted.   

I just applied to the patch to my local AVIARY snapshot and it seems to have
fixed the restart issue.

thanks.
(Assignee)

Updated

14 years ago
Attachment #154664 - Flags: superreview?(bzbarsky)
Comment on attachment 154664 [details] [diff] [review]
v1 patch

So let me see if I understand this right.  With this patch, if I change my
accept-language pref that will cancel all http transactions that are in
progress at the time?
(Assignee)

Comment 13

14 years ago
> So let me see if I understand this right.  With this patch, if I change my
> accept-language pref that will cancel all http transactions that are in
> progress at the time?

yes.  do you think that is unacceptable?

a more involved version would call a new method on nsHttpConnectionMgr with the
updated parameter (or maybe all parameters in one batch).  the conn mgr would
post an event to the socket thread containing the new parameter(s).  that event
would then set the new parameters on the conn mgr.  my fear is that the conn mgr
might get into a bad state if the parameters are reduced in value.  at first
glance, i don't think that will be the case ;-)
(Reporter)

Comment 14

14 years ago
Sorry, I am not a developer and thus my question may sound a bit dumb to you...
but what was actually the reason to change the behaviour of the conn mgr from Fx
0.8 to 0.9? Wouldn't it be possible to simply implement the connection
management as it was in 0.8?

Comment 15

14 years ago
Leaving it so that some of the connection prefs only take effect with a restart
seems acceptable to me, if the solution is otherwise complicated.

But that would bring us back to the problem you originally reported where the
prefs don't get picked up on startup - the connection management hasn't actually
changed to cause that bug.  I guess that problem could be fixed elsewhere (in
the profile loading stuff) instead?  Unless it's got fixed already (I haven't
tested, but that's what Remy said). 
something like this patch is still needed, so that changes of the prefs take
effect immediately, not only after a restart.

> might get into a bad state if the parameters are reduced in value.

hm.. ideally it would not care, leave those connections open and don't accept
new ones while there are more connections than configured :) or it could kill
"idle" connections (keep-alive with no current request).

Why would this need to happen via an event on the socket transport thread?
Couldn't it just acquire a lock and set the member vars?
(Assignee)

Comment 17

14 years ago
> something like this patch is still needed, so that changes of the prefs take
> effect immediately, not only after a restart.

agreed

> > might get into a bad state if the parameters are reduced in value.
> 
> hm.. ideally it would not care, leave those connections open and don't accept
> new ones while there are more connections than configured :) or it could kill
> "idle" connections (keep-alive with no current request).

"ideally" sure.. but reality bites sometimes, and we have to make sure we do not
introduce a regression ;-)


> Why would this need to happen via an event on the socket transport thread?
> Couldn't it just acquire a lock and set the member vars?

what lock is that?  multithreading is not so trivial... most of the conn mgr
runs on the socket thread free of having to synchronize with locks (which is a
huge benefit in that it greatly reduces the complexity of the conn mgr).  the
main thread communicates with the conn mgr via events posted to the socket
thread for most everything... we have a monitor for the purpose of implementing
synchronous shutdown, but that's about the only reason for a monitor in the conn
mgr code.
(Assignee)

Comment 18

14 years ago
Created attachment 154922 [details] [diff] [review]
v2 patch

OK, here's the "better" patch I spoke of.  The only downside to this patch is
that it adds a bit more code, but I think the fact that it won't cause
connections to close when prefs change makes it the right choice ;-)
Attachment #154664 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154664 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

14 years ago
Attachment #154922 - Flags: superreview?(bzbarsky)
Attachment #154922 - Flags: review?(cbiesinger)
Comment on attachment 154922 [details] [diff] [review]
v2 patch

+    nsresult UpdateParam(PRUint16 name, PRUint16 value);

why not use a named enum for the name param?

thanks for making this patch, I like this much more.
Attachment #154922 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 20

14 years ago
Created attachment 154957 [details] [diff] [review]
v2.1 patch

OK, with nsParamName...
Attachment #154922 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154922 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

14 years ago
Attachment #154957 - Flags: superreview?(bzbarsky)
[on cancelling connections on pref changes]
> yes.  do you think that is unacceptable?

Yes.  Consider downloads....

Looking at the patch now.
Comment on attachment 154957 [details] [diff] [review]
v2.1 patch

sr=bzbarsky.  I like this a lot more.
Attachment #154957 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 23

14 years ago
fixed-on-trunk for 1.8 alpha 3
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 24

14 years ago
There's yet another question in the Mozilla Firefox Support forum about this
situation so I'd like to nominate this bug for blocking aviary1.0.  
Flags: blocking-aviary1.0?

Comment 25

14 years ago
Remy: link? was that with a current branch build?  last time I tried with a
branch build, this worked for me.

Comment 26

14 years ago
(In reply to comment #25)
> Remy: link? was that with a current branch build?  last time I tried with a
> branch build, this worked for me.

Oops, sorry...
http://forums.mozillazine.org/viewtopic.php?t=114760

The problem with settings not being saved correctly was fixed in the FF branch
nightlies some time ago.  Darin's fix for the settings being internally locked
at their defaults currently isn't applied to the aviary branch though.  I've
been applying it to my own source tree for testing.
(Assignee)

Updated

14 years ago
Attachment #154957 - Flags: approval-aviary?

Comment 27

14 years ago
Comment on attachment 154957 [details] [diff] [review]
v2.1 patch

a=asa for branch checkin.
Attachment #154957 - Flags: approval-aviary? → approval-aviary+

Comment 28

14 years ago
Comment on attachment 154957 [details] [diff] [review]
v2.1 patch

a=asa for 1.7 checkin too
Attachment #154957 - Flags: approval1.7.x+
(Assignee)

Comment 29

14 years ago
fixed-aviary1.0
Keywords: fixed-aviary1.0
(Assignee)

Comment 30

14 years ago
fixed1.7.x
Keywords: fixed1.7.x

Updated

14 years ago
Flags: blocking-aviary1.0?
*** Bug 252903 has been marked as a duplicate of this bug. ***
*** Bug 264563 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.