Open Bug 1618934 Opened 5 years ago Updated 2 years ago

Convert IMAP protocol to NS_DispatchBackgroundTask

Categories

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

Tracking

(Not tracked)

People

(Reporter: rjl, Assigned: benc)

References

(Blocks 1 open bug, Regression)

Details

Attachments

(2 obsolete files)

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.

Example in bug 1618933.

Assignee: nobody → benc

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..)

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.

Blocks: 1586494
Priority: -- → P1
Attached patch 1618934-imap-comments-1.patch (obsolete) (deleted) — Splinter Review

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.

Attachment #9133496 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9133496 - Flags: review?(mkmelin+mozilla) → review+
Attached patch 1618934-imap-comments-2.patch (obsolete) (deleted) — Splinter Review
Attachment #9133496 - Attachment is obsolete: true
Attachment #9133729 - Flags: review+
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.

(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.

Attachment #9133729 - Attachment is obsolete: true

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?

Flags: needinfo?(benc)

(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.

Flags: needinfo?(benc)

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?
Priority: P1 → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: