1890448 - Rewrite folder compaction

Status: RESOLVED FIXED

(MailNews Core :: General, task, P1)

Description

[Ben Campbell]

The current folder compaction code is fiddly to deal with and hard to fix bugs in.
There was a big series of patches landed in Bug 1756520 which helped a bit by straightening out a lot of the surrounding scaffolding, but left the core compaction code mostly as-is.

A summary of the remaining problems:

• There are multiple code paths, dependent on folder type:
  • For IMAP folders it iterates through each message and gets the raw data using nsIMsgMessageService.streamMessage().
  • For local folders it issues a single up-front nsIMsgMessageService.copyMessages().
• Shonky error handling/recovery.
  • Cleanup/rollback is not always correct. A lot of functions just don't clean up after themselves if they fail.
  • There are multiple places where cleanup is attempted, and it's very hard to tell if there are cases which slip through the cracks.
• GUI code (progress/status updates, error alerts) is mixed in with the core logic.
• The code is over-complicated:
  • There's no separation of responsibility between msgStore, folder and database - The same code handles both database updates, folder updates and mbox rewriting.
  • IMAP expunge and db rebuild ("repair folder") are tangled up with the compaction.
  • Core compaction logic is complicated by handling multiple folders.
• It's not always clear when the "I'm finished" listener callback will be called.
  • It's an async operation, but the listener callback might be called synchronously - i.e. before the compact() function returns.

So this bug is a fresh attempt to sort it all out properly.

Comment 1

[Ben Campbell]

Attachment: Bug 1890448 - Rewrite folder compaction (1/2), Add nsIMsgPluggableStore.asyncCompact(). r=#thunderbird-reviewers

Comment 2

[Ben Campbell]

Attachment: Bug 1890448 - Rewrite folder compaction (2/2), Replace nsIMsgFolderCompactor. r=#thunderbird-reviewers

This patch replaces the old nsIMsgFolderCompactor code.
It provides new code which deals with the folder/db/notification/GUI
aspects around folder compaction, delegating the mbox wrangling to
nsIMsgPluggableStore.asyncCompact().

Depends on D207101

Comment 3

[Wayne Mery (:wsmwk)]

Do we need another bug to clean up preexisting tmp files in the profile directory?
Or use bug 1878541 for that purpose?

Comment 4

[Ben Campbell]

I'd probably use Bug 1878541 for that.
Once these patches land, nstmp files will no longer be used (well, there will still be temp files, but they are anonymous and the foldercompaction never really knows their name. And if anything goes wrong the file cleanup is waaaay more robust than the old compaction code).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4eedc45f960b
Rewrite folder compaction (1/2), Add nsIMsgPluggableStore.asyncCompact(). r=leftmostcat,mkmelin,darktrojan
https://hg.mozilla.org/comm-central/rev/dc0737b9d09a
Rewrite folder compaction (2/2), Replace nsIMsgFolderCompactor. r=darktrojan

Comment 6

[Wayne Mery (:wsmwk)]

So far ... no problem reports in bugzilla, support or topicbox.
Well, other than my bug 1898780

Comment 7

[Wayne Mery (:wsmwk)]

As discussed on Matrix, we will want to take this on tomorrow's beta.

Comment 8

[Wayne Mery (:wsmwk)]

Not taking this in 127.0b4 because of extra caution related to items just noted in bug 1898780

Comment 9

[Wayne Mery (:wsmwk)]

Comment on attachment 9395915 [details]
Bug 1890448 - Rewrite folder compaction (1/2), Add nsIMsgPluggableStore.asyncCompact(). r=#thunderbird-reviewers

[Approval Request Comment]
Approved for beta

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9396794 [details]
Bug 1890448 - Rewrite folder compaction (2/2), Replace nsIMsgFolderCompactor. r=#thunderbird-reviewers

[Approval Request Comment]
Approved for beta

Comment 11

[Daniel Darnell [:dandarnell]]

Thunderbird 127.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/5b31114107d5
https://hg.mozilla.org/releases/comm-beta/rev/8e2371878dbe

Comment 12

[Eugene Savitsky]

Is it in beta 5? In release notes is no word about it.

Comment 13

[Wayne Mery (:wsmwk)]

(In reply to Eugene Savitsky from comment #12)

Is it in beta 5? In release notes is no word about it.

yes