[PATCH 3 of 5] branchmap: acquires lock before writting the rev branch cache

[Pierre-Yves David]

On 08/07/2016 01:21 PM, Yuya Nishihara wrote:
> On Sat, 06 Aug 2016 01:02:55 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
>> # Date 1470401836 -7200
>> #      Fri Aug 05 14:57:16 2016 +0200
>> # Node ID 84885aeec2e442d18dcb70fcce2d65dd9fafbf91
>> # Parent  fa3f05a4219c4ea6b9bc9682f258e4418b36c265
>> # EXP-Topic vfsward
>> branchmap: acquires lock before writting the rev branch cache
>>
>> We now attempt to acquire a lock and write the branch cache within that lock.
>> This would prevent cache corruption when multiple processes try to write the cache
>> at the same time.
>
> (+CC Durham, Mads)
>
> I wonder if revbranchcache is designed to not take a lock. 9347c15d8136 says
> corruption can be recovered.

The lock taking is best effort, so I would say it is best to try to grab 
that lock. Even if corruption were recoverable it is better to avoid them.

>> --- a/mercurial/branchmap.py	Fri Aug 05 14:54:46 2016 +0200
>> +++ b/mercurial/branchmap.py	Fri Aug 05 14:57:16 2016 +0200
>> @@ -470,8 +470,10 @@ class revbranchcache(object):
>>      def write(self, tr=None):
>>          """Save branch cache if it is dirty."""
>>          repo = self._repo
>> -        if True:
>> +        wlock = None
>> +        try:
>>              if self._rbcnamescount < len(self._names):
>> +                wlock = repo.wlock(wait=False)
>>                  try:
>>                      if self._rbcnamescount != 0:
>>                          f = repo.vfs.open(_rbcnames, 'ab')
>> @@ -501,6 +503,7 @@ class revbranchcache(object):
>>
>>              start = self._rbcrevslen * _rbcrecsize
>>              if start != len(self._rbcrevs):
>> +                wlock = repo.wlock(wait=False)
>>                  revs = min(len(repo.changelog),
>>                             len(self._rbcrevs) // _rbcrecsize)
>>                  try:
>> @@ -521,3 +524,8 @@ class revbranchcache(object):
>>                                    inst)
>>                      return
>>                  self._rbcrevslen = revs
>> +        except error.LockHeld as inst:
>> +            repo.ui.debug("couldn't write revision branch cache: %s\n" % inst)
>> +        finally:
>> +            if wlock is not None:
>> +                wlock.release()
>
> wlock.held wouldn't be decremented appropriately if you'd take two wlocks.

Good catch, I'll send a V3.

Cheers,

-- 
Pierre-Yves David