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

RESOLVED FIXED in mozilla1.9.1

Status

MailNews Core
Networking: NNTP
RESOLVED FIXED
17 years ago
8 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: jcranmer)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
QA Contact: esther → stephend

Comment 1

17 years ago
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

16 years ago
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

16 years ago
This is a GNKSA blocking bug (19, abuse of the server, no more than 4 
connections). 
Blocks: 76449

Comment 4

16 years ago
As a GNKSA-related bug, it also blocks bug #12699.
Blocks: 12699

Comment 5

16 years ago
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).
Priority: -- → P2

Comment 6

16 years ago
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

16 years ago
Right; you queue up server requests which are handled in FIFO order using the 
available connections.
*** Bug 85739 has been marked as a duplicate of this bug. ***

Comment 9

16 years ago
Nominating.
Keywords: mozilla0.9.4, mozilla1.0

Updated

15 years ago
No longer blocks: 12699

Comment 10

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

Comment 11

15 years ago
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

14 years ago
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

14 years ago
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

14 years ago
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
Product: MailNews → Core

Comment 15

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

Updated

9 years ago
Duplicate of this bug: 301222
(Assignee)

Updated

9 years ago
Assignee: bienvenu → nobody
Priority: P2 → --
QA Contact: stephend → networking.news
Hardware: PC → All
(Assignee)

Updated

9 years ago
Duplicate of this bug: 67957
(Assignee)

Updated

9 years ago
Duplicate of this bug: 434769
(Assignee)

Updated

9 years ago
Blocks: 311774
(Assignee)

Comment 19

9 years ago
Taking this bug, as I need it to fix bug 311774 without overloading servers for most people.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9.1
(Assignee)

Updated

9 years ago
Duplicate of this bug: 288977
(Assignee)

Comment 21

9 years ago
Created attachment 344625 [details] [diff] [review]
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.
(Assignee)

Comment 22

9 years ago
Created attachment 347624 [details] [diff] [review]
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.
Attachment #344625 - Attachment is obsolete: true
Attachment #347624 - Flags: superreview?(bienvenu)
Attachment #347624 - Flags: review?(bienvenu)

Comment 23

9 years ago
Created attachment 352866 [details] [diff] [review]
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

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

Comment 25

9 years ago
Created attachment 353547 [details] [diff] [review]
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.
Attachment #347624 - Attachment is obsolete: true
Attachment #352866 - Attachment is obsolete: true
Attachment #353547 - Flags: superreview?(bienvenu)
Attachment #353547 - Flags: review?(bienvenu)
Attachment #347624 - Flags: superreview?(bienvenu)
Attachment #347624 - Flags: review?(bienvenu)

Comment 26

9 years ago
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 :-)
Attachment #353547 - Flags: superreview?(bienvenu)
Attachment #353547 - Flags: superreview+
Attachment #353547 - Flags: review?(bienvenu)
Attachment #353547 - Flags: review+
(Assignee)

Comment 27

9 years ago
Pushed as changeset 8d9d29ef8bac (with a fix to bienvenu's comment included).
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Depends on: 471131
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.