Closed Bug 66150 Opened 24 years ago Closed 16 years ago

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

Categories

(MailNews Core :: Networking: NNTP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: sspitzer, Assigned: jcranmer)

References

Details

Attachments

(1 file, 3 obsolete files)

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
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.
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.
This is a GNKSA blocking bug (19, abuse of the server, no more than 4 
connections). 
Blocks: 76449
As a GNKSA-related bug, it also blocks bug #12699.
Blocks: gnksa
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
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.
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. ***
Nominating.
No longer blocks: gnksa
*** Bug 189491 has been marked as a duplicate of this bug. ***
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).
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
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
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
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: bienvenu → nobody
Priority: P2 → --
QA Contact: stephend → networking.news
Hardware: PC → All
Blocks: 311774
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
Attached patch patch, version 0.8 (obsolete) — Splinter Review
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.
Attached patch Patch, version 1 (obsolete) — Splinter Review
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)
Attached patch unbit-rotted patch (obsolete) — Splinter Review
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 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);
Attached patch Patch, version 2Splinter Review
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 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+
Pushed as changeset 8d9d29ef8bac (with a fix to bienvenu's comment included).
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 471131
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.