Probably misunderstanding of the Threading/Lock code

[chris coleman]

On 19Sep2012 23:25, X Ryl 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.

Cameron Simpson wrote:
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.

.............................
Thanks to X and Cameron for finding this.

I agree with the upgrading to a Factory pattern here, replacing the current code which appears to be vulnerable to a race condition bug while OLI is running in multi threaded mode.  A race condition would possibly corrupt data in the LocalStatus, LocalStatusSQLite, and UIDMaps local databases/files.

While we're at it- why not upgrade UIDMaps to SQLite ?  Seems like it should be a higher quality solution than the flat file...

Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20120922/de2d6093/attachment-0003.html>