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) (deleted) — 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) (deleted) — 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) (deleted) — 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 2 (deleted) — Splinter 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.

Attachment

General

Created:
Updated:
Size: