218874 - Make IMAP use blocking reads

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David :Bienvenu]

Since the imap protocol code is on its own thread, it can use blocking reads
instead of using async reads and having necko proxy the data over with ODA
calls. It will simplify the code quite a bit, and should speed it up as well, on
a high speed network. I have this working in my tree. Patch upcoming

Comment 1

[David :Bienvenu]

Attachment: work in progress

this seems to work reasonably well, and gives a pretty good speedup on a LAN
connection

Comment 2

[David :Bienvenu]

I need to remove some of the commented out code.I think I can get rid of the
m_pump stuff too. We might want to look into using the synchronous cache methods
as well.

Comment 3

[David :Bienvenu]

Attachment: proposed fix

this patch is ready for review. I've tried SSL, connecting to a server that
doesn't accept connections, etc, and everything seems OK. Performance seems
better too.

Comment 4

[David :Bienvenu]

Comment on attachment 131257 [details] [diff] [review]
proposed fix

requesting reviews

Comment 5

[Scott MacGregor]

Comment on attachment 131257 [details] [diff] [review]
proposed fix

You commented out these two lines:

+//    if (m_chunkSize > m_maxChunkSize)
+//	 m_chunkSize = m_maxChunkSize;

are they save to outright remove or should we document why we think we can
comment them out?

Comment 6

[David :Bienvenu]

ah, good one - I'll just remove them and add a comment.

Comment 7

[Darin Fisher]

Comment on attachment 131257 [details] [diff] [review]
proposed fix

this also looks good to me.   looks like you also left GetThreadEventQueue
commented out in nsIImapProtocol, and that can probably be removed as well. 
i'm going to give this patch a whirl ;)

Comment 8

[David :Bienvenu]

great, thx, I've removed the commented out line from nsIImapProtocol.idl

Comment 9

[Darin Fisher]

ship it!  this thing feels sooo much faster!!

Comment 10

[David :Bienvenu]

fix checked in.

Comment 11

[Martin van Beurden]

From RFC 3501:
"Server data MAY be sent as a result of a client command, or MAY be sent
unilaterally by the server."
"A client MUST be prepared to accept any server response at all times. This
includes server data that was not requested."

This fix makes it impossible to implement features like the IDLE command (bug
141369) and what about autologout notifications from the server, these messages
won't be handled (if at all) by the client until the clients performs some kind
of action.

Comment 12

[David :Bienvenu]

this fix doesn't really change the way the imap parsing code was working,
actually. I believe idle connections could occasionally poll and see if there's
data to read from the socket, and if so, read it.

Comment 13

[Martin van Beurden]

I know the parsing was already done in a blocking way not using the async as
async so in that perspective this fix makes the code better. But in my opinion
all data from the server should be handled asynchronously (which would require a
probably too big a rewrite). Ah well, I guess I could live with polling although
it seems a bit unnessecary to me when you could be notified of data waiting to
be processed.

Comment 14

[Mats Palmgren (inactive)]

This caused regression bug 219185