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)
MailNews Core
Networking: NNTP
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: sspitzer, Assigned: jcranmer)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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•24 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•24 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•24 years ago
|
||
This is a GNKSA blocking bug (19, abuse of the server, no more than 4
connections).
Blocks: 76449
Comment 5•24 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•24 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•24 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 10•22 years ago
|
||
*** Bug 189491 has been marked as a duplicate of this bug. ***
Comment 11•22 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•22 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•21 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•21 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
Updated•20 years ago
|
Product: MailNews → Core
Comment 15•19 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•16 years ago
|
Assignee: bienvenu → nobody
Priority: P2 → --
QA Contact: stephend → networking.news
Hardware: PC → All
Assignee | ||
Comment 19•16 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 | ||
Comment 21•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 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•16 years ago
|
||
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•16 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•16 years ago
|
||
Pushed as changeset 8d9d29ef8bac (with a fix to bienvenu's comment included).
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•