Well, I took a bit of time to find out why it does get corrupted.<div>There is already a kind of "factory" for making such LocalStatus instance (look in repository/LocalStatus.py).</div><div>This factory is not protected by any lock, so the code like this:</div>
<div><div><div> def getfolder(self, foldername):</div><div> """Return the Folder() object for a foldername"""</div><div> if foldername in self._folders:</div><div> return self._folders[foldername]</div>
<div><br></div><div> folder = self.LocalStatusFolderClass(foldername, self)</div><div> self._folders[foldername] = folder</div><div> return folder</div></div><div><br></div><div>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).</div>
<div>This should not happen often, but it can happen.</div><div><br></div><div>Also, this part is even more wrong:</div><div> def makefolder(self, foldername):</div><div>[...]</div><div> # Invalidate the cache.</div>
<div> self._folders = {}</div></div><div><br></div><div>If any thread call makefolder, then the next getfolder call will re-create the LocalStatusFolder instance, and as such, will completely corrupt the filesystem.</div>
<div><br></div><div>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.</div>
<div><br></div><div>Then, when I get more understanding of the code, I can probably help fixing the root of the issue, instead of the effect.</div><div>Do you have a kind of "2mn tour inside the inner working of OFI" document somewhere ?</div>
<div><br></div><div>Cheers,</div><div>X</div><div><br></div><div><div class="gmail_quote">On Sat, Sep 22, 2012 at 12:52 AM, Cameron Simpson <span dir="ltr"><<a href="mailto:cs@zip.com.au" target="_blank">cs@zip.com.au</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 19Sep2012 23:25, X Ryl <<a href="mailto:boite.pour.spam@gmail.com">boite.pour.spam@gmail.com</a>> wrote:<br>
| While I was getting similar erroneous behaviour as reported in the list<br>
| previously, I've tried to track the issue in the code and found a major<br>
| issue.<br>
| In LocalStatus.py, LocalStatusSQLite.py,UIDMaps.py,<br>
| I'm seeing this pattern:<br>
|<br>
| class LocalStatusFolder(BaseFolder):<br>
| def __init__(self, name, repository):<br>
| [...]<br>
| self.savelock = threading.Lock()<br>
|<br>
| def save(self):<br>
| with self.savelock:<br>
|<br>
| I wonder the initial intend, but as I understand it, each INSTANCE of<br>
| the class will have its own lock/mutex, so the acquiring of the lock<br>
| in the method will only prevent mutual access if the same instance is<br>
| used in multiple thread.<br>
| That means that such code:<br>
| x1 = LocalStatusFolder(name, repo)<br>
| x2 = LocalStatusFolder(name, repo)<br>
| # In Thread 1<br>
| x1.save()<br>
| # In Thread 2<br>
| x2.save()<br>
|<br>
| is not protected (it's racy).<br>
|<br>
| I think the intend was to protect against all instance access to the<br>
| local status folder, that is, the "savelock" must be a static member<br>
| of the class.<br>
<br>
</div></div>No, it looks like it is to prevent simultaneous access to a folder of a<br>
particular name. Your solution (class variable) would prevent access to<br>
_any_ LocalStatusFolder at the same time.<br>
<br>
This code:<br>
<div class="im"><br>
x1 = LocalStatusFolder(name, repo)<br>
x2 = LocalStatusFolder(name, repo)<br>
<br>
</div>is the problem: if that _really_ occurs (create an object for the same<br>
LocalStatusFolder more than once) then LocalStatusFolder needs to become<br>
a factory function to return a previous instance if the name is reused.<br>
<br>
Cheers,<br>
<span class="HOEnZb"><font color="#888888">--<br>
Cameron Simpson <<a href="mailto:cs@zip.com.au">cs@zip.com.au</a>><br>
<br>
Yes, we plot no less than the destruction of the West. Just the other day a<br>
friend and I came up with the most pernicious academic scheme to date for<br>
toppling the West: He will kneel behind the West on all fours. I will push it<br>
backwards over him. - Michael Berube<br>
</font></span></blockquote></div><br></div>