Code review comment for lp:~vila/bzr/525571-lock-bazaar-conf-files

Revision history for this message
Robert Collins (lifeless) wrote :

Vila, I think I need to expand on this:

>> Perhaps that would be best tracked - when doing a read, if the object is not
>> locked, set self._can_write = False, and if it is, set it to True. Then check
>> that as well as whether it is locked, in _write_config.

> _can_write == is_locked() I see the idea but it won't change the end result nor the feedback we can give.

They are quite different.

If when you *read* you are unlocked, and you permit a *write* without forcing a new read, changes from other processes are discarded.

Here:
conf.read_something()
--client 2 writes to the file
conf.set_something()
--*boom* client 2's changes are lost

What we need is two fold: detection of this case (which setting 'can write' during *read* if it is write locked at the time will permit), and easy api use (which may be less immediate depending on impact).

Alternatively lock_write could possibly trigger a read after locking before returning, which would cause properly lock_write() guarded mutators to work correctly - as they would apply the mutation within the locked context.

In short - concurrency is hard, lets drink beer.

review: Needs Fixing

« Back to merge proposal