74518 - reading news messages (or attachments) from the memory cache leaves connections as "busy" causing us to open too many nntp connections

Status: VERIFIED FIXED

(MailNews Core :: Networking: NNTP, defect, P1)

Description

[Mike Young]

A while back there was a bug when attachments were downloaded multiple times:
once when the message was displayed, and once when the inline attachment was
being displayed. Now that the "too many connections" bugs have been cleared up,
and i can finally use news, this bug has become very noticable. 

Whenever someone tries to save a news attachment, that attachment is downloaded
from the server again. This would be a serious performance problem for people on
dialup connections. If we use the cache to hold the attachment while we view the
message (as was the case in the previous bug) why can't we use the cached copy
when we save the attachment. 

This should make news appear noticably faster, and will prevent any more
problems with "busy" connections.

This bug seems to olny affect when you save the attachment from the paperclip
button/menu. If you right click and save-as, you get a download window which
completes almost instantly (of course you lose the attachment name)

Also, shouldn't the saving attachment be done in a more obvious a manner than
simply starting the progress bar with "loading" ?

Comment 1

[Keyser Sose]

I think this is fixed when bug 73819 is checked in...thoughts?

Comment 2

[Mike Young]

I was not aware of bug 73819 but I don't think this will be fixed then either.
CCing [e-mail] since he is assigned the "attach cache to news" bug.
The problem in that bug is to convert all of the current cache uses over to the
new cache. This is just informing people that a new use exists for the cache. 

Lets not close this bug until we find out whether attachments will be saved from
the cache - new or otherwise.

Comment 3

[kyles]

Build ID: 2001052420, Windows 2000.

I can read articles no problem (re: bug 65927) but if I save an attachment, bam!
 After that, I
get the "too many open connections" error dialog when attempting to read another
article.  Saving the attachment is apparently using another connection and then
not releasing it.

I am using the right-click context menu and not the paper clip.

    - Kyle

Comment 4

[Keyser Sose]

Marking NEW.

Comment 5

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

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

Comment 6

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

updating.

not going through the cache is one thing, leaving open connections is another.

Comment 7

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

this is valid.  I was able to read 11 messages and save 11 attachments and end 
up with 11 new nsNNTPProtocol instances. 

debugging now...

Comment 8

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

interesting, saving the attachments doesn't cause us to spin up a new 
connection. 

right now, it looks like using a connection to save an attachment renders that 
connection "busy".

so, the next time we need a connection, all the ones in the connection cache 
are busy, so we create a new one.  (there's already another bug about changing 
the code so we queue up the requests we can't handle, instead of creating a new 
protocol instance.)

debugging more...

Comment 9

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

when reading from the cache, set the connection as busy, but never unset it 
when we are done.

testing a potential fix...

Comment 10

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

Attachment: patch, v1

Comment 11

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

so far, that patch address the problem of too many connections.

but, I'm noticing that after I save an attachment, we're closing the nntp 
connection (instead of keeping it open.)

I'll investigate that...

Comment 12

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

ah, I think I know it looked like we were closing the connection.

we're not updating the m_lastActiveTimeStamp on the connection when we reuse it 
(when getting the attachment out of the cache).

but that's correct, since we shouldn't be updating that unless we go over the 
wire and talk to the server.

I need to double check that we are not hitting the server when reading the 
attachment out of the cache.

so, when I was debugging, I was taking long enough so we'd hit the inactivity 
limit and rightly take the connection out of the connection cache.

I'll double check to make sure, and report back.

Comment 13

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

I may also see why we go to the network for the first attachment in the 
message, but not the second (if there is one.)

when we load the message, we ask the server for it by article num, not message 
id.

when we ask for the attachment, we ask for it by message id (and part #).  we 
need to make sure that even if we ask for the message by article num, we stuff 
it in the memory cache by id.

but I need to debug and verify.  that will be a seperate bug (about going to 
the server for attachments we've already downloaded.)

this bug will cover the problem of leaving open attachments.  tomorrow I'll 
verify my fix is correct.

Comment 14

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

this fix is not correct, I'm still seeing us open too many connections.

I'll continue to work on it tomorrow.

Comment 15

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

actually, it does address the problem.

I'm ending up with no more than 3 connections open at time, but there are a lot 
of connections that get opened, never get used, and get closed down.

I need to learn more about how the memory cache works.

I'll attach a new patch that is the old patch, plus some improvements to the 
NNTP logging that is helping me figure out what's going on.

Comment 16

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

Attachment: same fix, plus some changes for logging.

Comment 17

[scottputterman]

moving to 0.9.3

Comment 18

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

actually, we look protocol instances every time you read a message in the cache.

my initial fix (which is the same as my second patch, just no logging changes) 
is the right way to fix this.

moving back to 0.9.2, since fix in hand for a bad problem.

there are some other issues, but I'll log them as additional bugs.

Comment 19

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

updating summary.

Comment 20

[Scott MacGregor]

looks great to me. this definetly looks like a .0.9.2 stopper. Adding some more
keywords to get on PDt's radar too.

sr=mscott

Comment 21

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

the original bug (saving attachments doesn't go through the cache) is valid, 
I'll log up a new bug on that.

there's also another problem where we open the network socket too early, I'll 
log a new bug on that.

Comment 22

[Navin Gupta]

makes sense to me; r=naving

Comment 23

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

updating status

Comment 24

[chris hofmann]

a=chofmann

Comment 25

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

fixed.  I'll go open those other 2 bugs I mentioned.

Comment 26

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

spun off bugs #87344 and #87347

Comment 27

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

Over to my AT&T broadband account, where this could be verified from.

Comment 28

[Mike Young]

I believe I can verify this fixed, as I can save news messages without getting
that dreaded "too many connections" dialog. However, I am CCing [e-mail]
since he also saw this bug. 

I'm marking it verified, but if anyone sees saving messages (or attachments)
leaving connections as busy, please reopen. 

Remember that saving the attachments themselves requires a new connection (bug
87344) and just viewing attahments from the cache gets another connection as
well (bug 87347) 

At least now they're not left as busy :-)