66150 - don't allow more than N open nntp connections to a server

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[(not reading, please use [e-mail] instead)]

bienvenu wrote: 

"We should look at fixing the news protocol state machine to not allow
more than N open connections (i.e., closing connections instead of leaving them
in the cache if the cache is full)."

david, feel free to re-assign this to me if you don't want it.

Comment 1

[Stephen Ostermiller]

My server limits clients to three connections and then gives an error message. 
I get many many error messages when using news groups because of this.

Comment 2

[Mike Young]

This is very closely related to bug 65927 [News leaves connections to server open]

I don't know whether this is dependant on that bug, or a duplicate.

Stephen, the problem you are seeing is that bug.

If someone wants to solve these two bugs by making a pref that sets a number of
news connections, that would work, but i'd rather see a working net-cache mechanism.

Comment 3

[John Moreno]

This is a GNKSA blocking bug (19, abuse of the server, no more than 4 
connections). 

Comment 4

[Garth Wallace]

As a GNKSA-related bug, it also blocks bug #12699.

Comment 5

[David :Bienvenu]

And what do we do if all the connections are taken? Just fail and tell the user
to shutdown the app? This problem is almost always because of a client bug
(i.e., we're not trying to open extra connections - in theory, we should only
ever open one or two).

Comment 6

[Mike Young]

In the old days... (NS4.x) Netscape would kill all old connections if you go 
onto a new news message. In Mozilla, you can click on a message and click on a 
new one while it downloads. Instead of stopping the download on the previous 
message, Mozilla starts another connection and downloads the new one AT THE SAME 
TIME. As you can imagine, this creates "Busy" connections that will result in 
many many new connections being made instead of being reused. 

Outlook/Outlook express don't seem to have these problems. From what I can tell, 
with them you have 3 connections at a time. You click on a message and can click 
on as many new essages as you want. Then, 3 at a time, it goes through and 
downloads the messages you clicked in order. This behavior is interesting, but I 
don't know whether it is the right thing to do. It is however a better idea that 
giving the user an ugly message, or cutting off their (possibly in use) old 
connections that we want to shut down.

We need to figure out what Mozilla should do in this situation. It really 
becomes a problem with people on 56k and lower connections. If someone 
interrupts a message partially downloaded, mozilla needs to either stop the 
connections, or throttle the number allowwed. 
The Number of connections is this bug...
Any other behavior would require a new bug.

Comment 7

[Simon Fraser [no longer active]]

Right; you queue up server requests which are handled in FIFO order using the 
available connections.

Comment 8

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 85739 has been marked as a duplicate of this bug. ***

Comment 9

[Håkan Waara]

Nominating.

Comment 10

[Jo Hermans]

*** Bug 189491 has been marked as a duplicate of this bug. ***

Comment 11

[Gábor Lipták]

answering for http://bugzilla.mozilla.org/show_bug.cgi?id=189491

currently 3 but I'm also running another app accessing the newsfeed.

This mostly happens when starting up the mail client and clicking on the news
server, than clicking on particular groups to read (while updating the newsgroup
counts did not finish yet).

Comment 12

[Gábor Lipták]

this seems to have gotten worse with

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030325

i'm seeing several nntp connections "stuck"

TCP    win:1279             newsfeed.com:nntp  ESTABLISHED
TCP    win:1281             newsfeed.com:nntp  ESTABLISHED
TCP    win:1284             newsfeed.com:nntp  ESTABLISHED
TCP    win:1289             newsfeed.com:nntp  ESTABLISHED

until i didn't kill the mail client

Comment 13

[Gábor Lipták]

i noticed another detail with 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030507

more than N connections message also pops up, when one clicks on a "binary"
(with an attachement) message in a non-binary group, and than clicks on the next
message

Comment 14

[[e-mail]]

More "stuck" connections :-(

Foreign Address        State
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
204.29.187.156:119     ESTABLISHED
194.168.4.93:119       ESTABLISHED
204.29.187.156:119     ESTABLISHED
207.200.81.212:6667    ESTABLISHED
192.108.102.201:143    ESTABLISHED
192.108.102.201:143    ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED
194.168.4.93:119       ESTABLISHED

Comment 15

[David Jameson]

I think the problem isn't so much that we need to limit the number of connections, but that mozilla opens up a completely new connection even when there are x unused NNTP connections open already.

Comment 19

[Joshua Cranmer [:jcranmer]]

Taking this bug, as I need it to fix bug 311774 without overloading servers for most people.

Comment 21

[Joshua Cranmer [:jcranmer]]

Attachment: patch, version 0.8

This is nearly complete in terms of a patch, but there a few things left to do:
1. I think I get some new assertions here, but I need to verify...
2. Tests would be nice.

Comment 22

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 1

This patch:

1. Adds some tests (a test, to be precise).
2. Makes the connection attachment process more tolerant of errors.
3. Documents new functions and some related old ones.

Comment 23

[David :Bienvenu]

Attachment: unbit-rotted patch

this is an un-bitrotted patch. I've been running with it for a couple days. The one thing I've noticed is that a lot of news db's are getting held open. I don't know if this patch could possibly be related - I only noticed it because I've been trying to convince imap auto sync not to hold open a bunch of db's...

Comment 24

[David :Bienvenu]

Comment on attachment 347624 [details] [diff] [review]
Patch, version 1

m_queuedUri's is really a queue of channels that correspond to uri's we want to run, so that variable name should probably change.

Other than that, it looks OK. I'm a little worried about potential cycles with the mock channel because we've had that problem with imap, so you might want to think about that a little...

Why are these macros better than just saying protocol.SetLoadGroup(m_loadGroup), etc.?

+#define ATTACH_VAR(attribute, variable) \
+  protocol.Set##attribute(variable);
+
+  ATTACH_VAR(LoadGroup, m_loadGroup);
+  ATTACH_VAR(LoadFlags, m_loadFlags);
+  ATTACH_VAR(OriginalURI, m_originalUrl);
+  ATTACH_VAR(Owner, m_owner);
+  ATTACH_VAR(NotificationCallbacks, m_notificationCallbacks);
+  ATTACH_VAR(ContentType, m_contentType);

Comment 25

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 2

Sorry about the patch application problem.

I can now confirm that there are no cycles in common cases (i.e., loading a few dozen messages at once). The only place that the leaks could occur is if we have a queue of waiting URIs on shutdown, one of which holds a reference to the server, or if the server is holding a reference to a channel that's enqueued (again, at shutdown). None of the URIs I've been poking out have a reference to the server, so it all looks OK.

Comment 26

[David :Bienvenu]

Comment on attachment 353547 [details] [diff] [review]
Patch, version 2

r/sr=me, a couple nits about comments:

this isn't quite right - it's not ourself, but the passed in protocol:

+  // Now set the protocol to ourselves
+  m_protocol = &protocol;

and you might comment that m_protocol doesn't hold a reference to the protocol, and describe the ownership model that makes this OK :-)

Comment 27

[Joshua Cranmer [:jcranmer]]

Pushed as changeset 8d9d29ef8bac (with a fix to bienvenu's comment included).