Probably misunderstanding of the Threading/Lock code

[X Ryl]

Well, I took a bit of time to find out why it does get corrupted.
There is already a kind of "factory" for making such LocalStatus instance
(look in repository/LocalStatus.py).
This factory is not protected by any lock, so the code like this:
    def getfolder(self, foldername):
        """Return the Folder() object for a foldername"""
        if foldername in self._folders:
            return self._folders[foldername]

        folder = self.LocalStatusFolderClass(foldername, self)
        self._folders[foldername] = folder
        return folder

Can get corrupted if two threads call the method at the same time (both
thread can test for existence of the foldername inside the _folders member,
and both can create a local instance of LocalStatusFolder for it, provided
any thread is interrupted/scheduled after the dictionnay lookup).
This should not happen often, but it can happen.

Also, this part is even more wrong:
    def makefolder(self, foldername):
[...]
        # Invalidate the cache.
        self._folders = {}

If any thread call makefolder, then the next getfolder call will re-create
the LocalStatusFolder instance, and as such, will completely corrupt the
filesystem.

I'm continuing to dig inside the code to understand where/when the thread
are created and used, but I think, as a temporary fix, I would put the
LocalStatusFolder's Lock as a class instance so all threads will be
serialized when touching the filesystem, thus preventing Maildir corruption
like it's the case currently.

Then, when I get more understanding of the code, I can probably help fixing
the root of the issue, instead of the effect.
Do you have a kind of "2mn tour inside the inner working of OFI" document
somewhere ?

Cheers,
X

On Sat, Sep 22, 2012 at 12:52 AM, Cameron Simpson <cs at zip.com.au> wrote:

> On 19Sep2012 23:25, X Ryl <boite.pour.spam at gmail.com> wrote:
> |   While I was getting similar erroneous behaviour as reported in the list
> | previously, I've tried to track the issue in the code and found a major
> | issue.
> | In LocalStatus.py, LocalStatusSQLite.py,UIDMaps.py,
> | I'm seeing this pattern:
> |
> | class LocalStatusFolder(BaseFolder):
> |     def __init__(self, name, repository):
> |     [...]
> |         self.savelock = threading.Lock()
> |
> |     def save(self):
> |         with self.savelock:
> |
> | I wonder the initial intend, but as I understand it, each INSTANCE of
> | the class will have its own lock/mutex, so the acquiring of the lock
> | in the method will only prevent mutual access if the same instance is
> | used in multiple thread.
> | That means that such code:
> | x1 = LocalStatusFolder(name, repo)
> | x2 = LocalStatusFolder(name, repo)
> | # In Thread 1
> | x1.save()
> | # In Thread 2
> | x2.save()
> |
> | is not protected (it's racy).
> |
> | I think the intend was to protect against all instance access to the
> | local status folder, that is, the "savelock" must be a static member
> | of the class.
>
> No, it looks like it is to prevent simultaneous access to a folder of a
> particular name. Your solution (class variable) would prevent access to
> _any_ LocalStatusFolder at the same time.
>
> This code:
>
>   x1 = LocalStatusFolder(name, repo)
>   x2 = LocalStatusFolder(name, repo)
>
> is the problem: if that _really_ occurs (create an object for the same
> LocalStatusFolder more than once) then LocalStatusFolder needs to become
> a factory function to return a previous instance if the name is reused.
>
> Cheers,
> --
> Cameron Simpson <cs at zip.com.au>
>
> Yes, we plot no less than the destruction of the West. Just the other day a
> friend and I came up with the most pernicious academic scheme to date for
> toppling the West: He will kneel behind the West on all fours. I will push
> it
> backwards over him.     - Michael Berube
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20120924/0b09d620/attachment-0003.html>