1618934 - Convert IMAP protocol to NS_DispatchBackgroundTask

Status: NEW

(MailNews Core :: Networking: IMAP, task, P3)

Description

[Rob Lemley [:rjl]]

Bug 1613440 deprecates use of NS_NewNamedThread. The suggest migration is to use NS_DispatchBackgroundTask or NS_CreateBackgroundTaskQueue.

More details and some examples can be found in this PSA:
https://groups.google.com/forum/#!msg/mozilla.dev.platform/SwmYPy_8_8k/2vswXjUpBgAJ

In the meantime, the "IMAP" thread name is being added to ThreadAllow.txt.

Comment 1

[Magnus Melin [:mkmelin]]

Example in bug 1618933.

Comment 2

[Ben Campbell]

Been picking my way through this one today - just wanted to jot down some notes to clarify my thinking on it so far.
It's a little more involved than the libical one as the thread stays in a long-running loop, rather than just kicking off one-shot jobs. We don't want to tie up the background thread(s) with a loop.
The loop essentially just waits until ready for the the next url, so NS_CreateBackgroundTaskQueue should be a pretty natural replacement - there are a couple of places which signal the thread to check for the next url, so instead they'll just submit new background tasks directly.
There's a little complication (imap IDLE is also handled in the loop), but that's my general plan so far.
(I suspect doing this might also help shed some light on Bug 1586494 - even just by virtue of emerging with a better understanding of the IMAP threading..)

Comment 3

[Wayne Mery (:wsmwk)]

Sounds promising.

Given that Magnus marked bug 1586494 P1, I'm setting that here as well. If that is not accurate feel free to change.

Comment 4

[Ben Campbell]

Attachment: 1618934-imap-comments-1.patch

Been digging through the IMAP code, so here's a stab at trying to flesh out the comments of various classes and interfaces as I go along. Likely to be more of these.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9133496 [details] [diff] [review]
1618934-imap-comments-1.patch

Review of attachment 9133496 [details] [diff] [review]:
-----------------------------------------------------------------

Comments look good

::: mailnews/imap/public/nsIImapMailFolderSink.idl
@@ +30,5 @@
>     const long kFailedMove = 9;
>  };
>  
> +/**
> + * This is the interface the IMAP system uses to communicate with the local

nit: please write nsIImapMailFolderSink instead of this. The same goes in for the other comments.

::: mailnews/imap/src/nsImapIncomingServer.h
@@ +107,5 @@
>                            nsCString &existingUri);
>  
>    nsCOMArray<nsIImapProtocol> m_connectionCache;
> +
> +  // All requests waiting for a real connection.

For all documentation comments, please use /** */ style so we could make generated documentation from it.

::: mailnews/imap/src/nsImapProtocol.h
@@ +171,5 @@
> + * When an IMAP routine calls a member function of one of these sink proxies,
> + * it dispatches a call to the real sink object on the main thread, then
> + * blocks until the call is completed.
> + *
> + */

nit: remove the extra blank line

@@ -364,5 @@
>    nsCString m_serverKey;
>    nsCString m_realHostName;
>    char *m_dataOutputBuf;
>    RefPtr<nsMsgLineStreamBuffer> m_inputStreamBuffer;
> -  uint32_t m_allocatedSize;  // allocated size

this is not used yes, but looks like it's still referred to in the .cpp code

::: mailnews/imap/src/nsImapService.h
@@ +19,5 @@
>  class nsIImapUrl;
>  class nsIMsgFolder;
>  class nsIMsgIncomingServer;
>  
> +/* nsImapService implements the IMAP protocol.

/**
 * nsImapService implements the IMAP protocol.

Comment 6

[Ben Campbell]

Attachment: 1618934-imap-comments-2.patch

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9133729 [details] [diff] [review]
1618934-imap-comments-2.patch

Review of attachment 9133729 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/src/nsImapProtocol.h
@@ +406,5 @@
>  
>    nsCOMPtr<nsIAsyncInputStream> m_channelInputStream;
>    nsCOMPtr<nsIAsyncOutputStream> m_channelOutputStream;
> +
> +  /** The currently-running request. */

Hmm, there is no hyphen between the adverb and the gerund.
https://www.dailywritingtips.com/adverbs-and-hyphens/
Compounds formed by an adverb ending in ly plus an adjective or participle (such as largely irrelevant or smartly dressed) are not hyphenated either before or after a noun, since ambiguity is virtually impossible.

Comment 8

[Ben Campbell]

(In reply to Jorg K (GMT+1) from comment #7)

Comment on attachment 9133729 [details] [diff] [review]
::: mailnews/imap/src/nsImapProtocol.h

• /** The currently-running request. */

Hmm, there is no hyphen between the adverb and the gerund.

Gah, yes - a grammatical tick of mine!

Fixed up and moved over to Bug 1622979.

Comment 9

[Wayne Mery (:wsmwk)]

Ben, would it be helpful to have this patched before making more intrusive/risky changes that are needed to resolve our various longstanding imap threading bugs?

Comment 10

[Ben Campbell]

(In reply to Wayne Mery (:wsmwk) from comment #9)

Ben, would it be helpful to have this patched before making more intrusive/risky changes that are needed to resolve our various longstanding imap threading bugs?

I don't know for sure, but I think it won't make any difference. Whatever happens in this bug, we're still running all the IMAP protocol code on a non-main thread, so all the same sync issues remain.

I've a suspicion that my thoughts at the time of comment #2 were a bit naive. The approach still sounds feasible, but it troubles me that we'd be going from a nice simple single-blockable-thread-of-execution model a to more complicated dispatch system, but without going to the full state-machine approach that the IMAP thread was aiming to avoid. So, more complexity. And it's not like we don't already have enough complexity in IMAP :-). But I think it's just going to be a matter of having a go at at, and being prepared to chuck it away if it adds too much hassle.

Comment 11

[Wayne Mery (:wsmwk)]

Right, I wasn't suggesting this will in any way fix any aspect of imap sync. Rather I was curious if it might make help development/debugging of the imap fixes, and then after shipping help with the inevitable issues that will come.

Parallel ideas:

1. we don't know when they will actually remove the current code, which could force to adopt this at an inconvenient time in our own development process - there's nothing imminent in any of the blocking bugs, but that can always change. And as you stated our threading is complex, so we wouldn't want to risk being forced to land this late in an esr development cycle.
2. as you indicate this may complicate fixing the imap sync problems, so is it preferable to do these at the same time as one package, or do them at different times, and if so in which order?