Probably misunderstanding of the Threading/Lock code
christocoleman at yahoo.com
Sun Sep 23 04:27:16 BST 2012
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
| 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
| # In Thread 2
| 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.
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...
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the OfflineIMAP-project