Differential D12019

make sure to create unique filename in maildir resource
ClosedPublic
Actions

Authored by mkoller on Apr 7 2018, 12:16 PM.

Details

Reviewers
mlaurent
dvratil
Commits
R44:e681e74f6a4e: make sure to create unique filename in maildir resource
R44:ba213f330650: make sure to create unique filename in maildir resource
Summary

While having akonadiconsole open by chance, I saw a few error messages like:

AgentBase(akonadi_maildir_resource_0): Could not move message '1521037293.R521.lapi.koller' from '/home/koller/Mail/inbox' to '/home/koller/Mail/.z_div. privat.directory/KDE-FW-Devel'. The error was Destination file exists.

and in fact this is correct - I really have some mail filenames with the same name.
I assume that this can happen when during maildownload (POP3) and filtering a mail from the inbox is moved to the filter target folder
and within the same second a new mailfile shall be created. In my case both files come from the same mailinglist.
(why also the random part is then the same seems just to be bad luck).

This patch changes the "current-time + random" way to use QUuid - which from the source seems it was the original idea but due to a Qt bug in an old Qt version, it was not used
https://bugreports.qt.io/browse/QTBUG-8374

Since this bug is solved since Qt 4.7, I feel this patch is a much safer approach to generate a unique file name.

Diff Detail

Repository
R44 KDE PIM Runtime
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mkoller created this revision.Apr 7 2018, 12:16 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 7 2018, 12:16 PM
Restricted Application added a subscriber: KDE PIM. · View Herald Transcript
mkoller requested review of this revision.Apr 7 2018, 12:16 PM
dvratil added a subscriber: dvratil.Apr 9 2018, 9:22 AM

I wonder if this won't make the filenames unnecessarily long. I think that just simply switching to QDateTime::currentMSecsSinceEpoch() would give us enough granularity. There's basically no chance that createUniqueFileName() would be called twice in a single millisecond, and even if so, the chance of two consecutive qrand()s generating the same pseudorandom number reduces the chance of a conflict even further.

maildir.cpp
552–554 ↗(On Diff #31584)

I would suggest

auto uuid = QUuid::createUuid().toString();
uuid[uuid.size() - 1] = '.'
return uuid.mid(1);

to avoid unnecessary string copying.

mkoller updated this revision to Diff 31754.Apr 9 2018, 5:18 PM

changed to milliSeconds.
Altough I'm not really sure if this is a good solution, since how can we know how fast computers will be in some years ?
And QUuid is there for exactly this reason - to generate a unique name.
Yes, the filenames are twice as long - altough: who cares ? Do you think it hurts somewhere ?

I assume the rand seed init is not needed, since it does not matter what random numbers we get, and the comment
suggest that it was only there for QUuid (which was not used anyways...)

winterz added a subscriber: winterz.Apr 9 2018, 7:34 PM
In D12019#243301, @mkoller wrote:

Yes, the filenames are twice as long - altough: who cares ? Do you think it hurts somewhere ?

Depends on the filesystem. I believe we had the situation on Windows where the filename length was limited to 260 chars.
In Windows 10, there is a registry setting for filename length which I suppose would eliminate that problem for your more sophisticated users.

knauss added a subscriber: knauss.Apr 10 2018, 5:57 PM

But the whole createUniqueFileName is not supposed to generate unique names anyways, as it is used in a do loop in Maildir::addEntry. I think be better approach to add to mutex, to garantee
QFile::exists(key) || QFile::exists(finalKey) || QFile::exists(curKey) is valid. Maybe we should touch the file in tmp dir immediately. And than it is irrelevant if we use a not so good createUniqueFileName.

mkoller added a comment.Apr 10 2018, 7:32 PM
In D12019#243886, @knauss wrote:

But the whole createUniqueFileName is not supposed to generate unique names anyways, as it is used in a do loop in Maildir::addEntry. I think be better approach to add to mutex, to garantee
QFile::exists(key) || QFile::exists(finalKey) || QFile::exists(curKey) is valid. Maybe we should touch the file in tmp dir immediately. And than it is irrelevant if we use a not so good createUniqueFileName.

It IS supposed to and SHOULD create UNIQUE file names, since it does not matter that the mail file in ONE folder are unique when the file shall be moved into a different folder due to a filter rule afterwards.
This is exactly the problem I encounter. mails from the inbox shall be moved into some other folder but that fails since there is already the exact same file existing in the new target folder.
All this can be avoided when the files are unique in the first place, and unique all over all mail dirs

dvratil accepted this revision.Apr 11 2018, 2:17 PM

@knauss but the maildir is not multithreaded, is it, so mutex wouldn't fix anything.

Technically nothing prevents the maildir resource from being fast (and unlucky wrt to randomness) enough to generate two identical filenames for two emails in different folders.

If Martin is correct and the issue is indeed caused by the mailfilter moves, then the problem is obviously caused simply by Maildir::moveEntryTo() reusing the old filename when moving an email from folder A to folder B. By increasing the timestamp granularity we reduce the chance of such filename conflict in the future (it won't affect existing emails, so the problem can still occur on old emails).

This revision is now accepted and ready to land.Apr 11 2018, 2:17 PM
Closed by commit R44:e681e74f6a4e: make sure to create unique filename in maildir resource (authored by mkoller). · Explain WhyApr 11 2018, 2:50 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 31890

View Options

resources/maildir/libmaildir/maildir.cpp

Loading...