Comment 37 for bug 2060534

Revision history for this message
In , Omry Yadan (omry) wrote :

> See Bug 1890448. There's a patch in phabricator attached there.
Thanks.

> > 2. Making sure that multiple processes/threads would not write to the same tmp file concurrently.
>
> There is some folder locking, but nothing at the FS level. All folder stuff is on the main thread anyway (so the JS frontend can use it).
Is it? There is a progress bar on compaction and the UI is not locked as far as I remember.

As for multiple processes:
It looks like attempting to open TB a second time focuses on the first window instead of opening a new instance so this is probably something safe to ignore and in unlikely to be related anyway.

> > 4. Clean up compaction tmp files on startup on case of an unclean shutdown during compaction.
>
> Nope... but that's mostly covered by 3. A hard power-down or extreme crash would probably leave something behind, I think...
> Potentially could add a startup check, but there are some corner-case issues there too.

What I have done before in cases like this was to drop a pid file in the folder, and check it on startup.
If the PID inside the pid file is different than my process pid and if the file was not touched recently (per file system metadata) I assume it's a stale file and clean it up (along with the old pid file).

> > 5. Ensure that the number of written messages while compacting does not exceed the number of messages in the folder being compacted.
>
> Not a problem with the new code. The store iterates through the existing messages as it writes out the new mbox, and never revisits ones it's already written or skipped.

I suggested it as a simple fallback protection mechanism. If the file grows indefinitely the problem is likely either an infinite stream of messages coming while iterating (another possibility is very large messages being written due to some bug).
We can punt this for now as a total rewrite like yours have a good chance of working out the underlying bug anyway.