Patchwork [2,of,2] tags: take lock while writing `tags2` cache

login
register
mail settings
Submitter Pulkit Goyal
Date Sept. 11, 2020, 7:04 a.m.
Message ID <3eae3d3a1b44b0fe10e0.1599807841@workspace>
Download mbox | patch
Permalink /patch/47129/
State Accepted
Headers show

Comments

Pulkit Goyal - Sept. 11, 2020, 7:04 a.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1599727895 -19800
#      Thu Sep 10 14:21:35 2020 +0530
# Node ID 3eae3d3a1b44b0fe10e00766e1113c50d48ed086
# Parent  3095d36efcaf9ce835249c9312e892b0d54aac78
# EXP-Topic tags-fix
tags: take lock while writing `tags2` cache

Before this patch, we were not taking lock while writing the cache. This cache
is shared one and hence multiple writes can happen at same time. So let's take a
lock before writing it. We don't wait for lock because outdated cache is still
fine.
Yuya Nishihara - Sept. 12, 2020, 2:25 a.m.
On Fri, 11 Sep 2020 12:34:01 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1599727895 -19800
> #      Thu Sep 10 14:21:35 2020 +0530
> # Node ID 3eae3d3a1b44b0fe10e00766e1113c50d48ed086
> # Parent  3095d36efcaf9ce835249c9312e892b0d54aac78
> # EXP-Topic tags-fix
> tags: take lock while writing `tags2` cache
> 
> Before this patch, we were not taking lock while writing the cache. This cache
> is shared one and hence multiple writes can happen at same time. So let's take a
> lock before writing it. We don't wait for lock because outdated cache is still
> fine.
> 
> diff --git a/mercurial/tags.py b/mercurial/tags.py
> --- a/mercurial/tags.py
> +++ b/mercurial/tags.py
> @@ -514,38 +514,47 @@ def _getfnodes(ui, repo, nodes):
>  
>  def _writetagcache(ui, repo, valid, cachetags):
>      filename = _filename(repo)
> +
>      try:
> -        cachefile = repo.cachevfs(filename, b'w', atomictemp=True)

Write race should be dealt with atomictemp. This wouldn't need a lock.
Pulkit Goyal - Sept. 14, 2020, 8:09 a.m.
On Sat, Sep 12, 2020 at 8:31 AM Yuya Nishihara <yuya@tcha.org> wrote:
>
> On Fri, 11 Sep 2020 12:34:01 +0530, Pulkit Goyal wrote:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1599727895 -19800
> > #      Thu Sep 10 14:21:35 2020 +0530
> > # Node ID 3eae3d3a1b44b0fe10e00766e1113c50d48ed086
> > # Parent  3095d36efcaf9ce835249c9312e892b0d54aac78
> > # EXP-Topic tags-fix
> > tags: take lock while writing `tags2` cache
> >
> > Before this patch, we were not taking lock while writing the cache. This cache
> > is shared one and hence multiple writes can happen at same time. So let's take a
> > lock before writing it. We don't wait for lock because outdated cache is still
> > fine.
> >
> > diff --git a/mercurial/tags.py b/mercurial/tags.py
> > --- a/mercurial/tags.py
> > +++ b/mercurial/tags.py
> > @@ -514,38 +514,47 @@ def _getfnodes(ui, repo, nodes):
> >
> >  def _writetagcache(ui, repo, valid, cachetags):
> >      filename = _filename(repo)
> > +
> >      try:
> > -        cachefile = repo.cachevfs(filename, b'w', atomictemp=True)
>
> Write race should be dealt with atomictemp. This wouldn't need a lock.

Missed it thanks.

Patch

diff --git a/mercurial/tags.py b/mercurial/tags.py
--- a/mercurial/tags.py
+++ b/mercurial/tags.py
@@ -514,38 +514,47 @@  def _getfnodes(ui, repo, nodes):
 
 def _writetagcache(ui, repo, valid, cachetags):
     filename = _filename(repo)
+
     try:
-        cachefile = repo.cachevfs(filename, b'w', atomictemp=True)
-    except (OSError, IOError):
+        lock = repo.lock(wait=False)
+    except error.LockError:
+        repo.ui.log(
+            b'tagscache',
+            b'not writing .hg/cache/%s because lock cannot be acquired\n'
+            % filename,
+        )
         return
 
-    ui.log(
-        b'tagscache',
-        b'writing .hg/cache/%s with %d tags\n',
-        filename,
-        len(cachetags),
-    )
-
-    if valid[2]:
-        cachefile.write(
-            b'%d %s %s\n' % (valid[0], hex(valid[1]), hex(valid[2]))
+    try:
+        cachefile = repo.cachevfs(filename, b'w', atomictemp=True)
+        ui.log(
+            b'tagscache',
+            b'writing .hg/cache/%s with %d tags\n',
+            filename,
+            len(cachetags),
         )
-    else:
-        cachefile.write(b'%d %s\n' % (valid[0], hex(valid[1])))
+
+        if valid[2]:
+            cachefile.write(
+                b'%d %s %s\n' % (valid[0], hex(valid[1]), hex(valid[2]))
+            )
+        else:
+            cachefile.write(b'%d %s\n' % (valid[0], hex(valid[1])))
 
-    # Tag names in the cache are in UTF-8 -- which is the whole reason
-    # we keep them in UTF-8 throughout this module.  If we converted
-    # them local encoding on input, we would lose info writing them to
-    # the cache.
-    for (name, (node, hist)) in sorted(pycompat.iteritems(cachetags)):
-        for n in hist:
-            cachefile.write(b"%s %s\n" % (hex(n), name))
-        cachefile.write(b"%s %s\n" % (hex(node), name))
+        # Tag names in the cache are in UTF-8 -- which is the whole reason
+        # we keep them in UTF-8 throughout this module.  If we converted
+        # them local encoding on input, we would lose info writing them to
+        # the cache.
+        for (name, (node, hist)) in sorted(pycompat.iteritems(cachetags)):
+            for n in hist:
+                cachefile.write(b"%s %s\n" % (hex(n), name))
+            cachefile.write(b"%s %s\n" % (hex(node), name))
 
-    try:
         cachefile.close()
     except (OSError, IOError):
         pass
+    finally:
+        lock.release()
 
 
 def tag(repo, names, node, message, local, user, date, editor=False):
diff --git a/tests/test-tags.t b/tests/test-tags.t
--- a/tests/test-tags.t
+++ b/tests/test-tags.t
@@ -163,7 +163,7 @@  Failure to acquire lock results in no wr
   1970/01/01 00:00:00 bob @b9154636be938d3d431e75a7c906504a079bfe07 (5000)> identify
   1970/01/01 00:00:00 bob @b9154636be938d3d431e75a7c906504a079bfe07 (5000)> not writing .hg/cache/hgtagsfnodes1 because lock cannot be acquired
   1970/01/01 00:00:00 bob @b9154636be938d3d431e75a7c906504a079bfe07 (5000)> 0/2 cache hits/lookups in * seconds (glob)
-  1970/01/01 00:00:00 bob @b9154636be938d3d431e75a7c906504a079bfe07 (5000)> writing .hg/cache/tags2-visible with 1 tags
+  1970/01/01 00:00:00 bob @b9154636be938d3d431e75a7c906504a079bfe07 (5000)> not writing .hg/cache/tags2-visible because lock cannot be acquired
   1970/01/01 00:00:00 bob @b9154636be938d3d431e75a7c906504a079bfe07 (5000)> identify exited 0 after * seconds (glob)
   1970/01/01 00:00:00 bob @b9154636be938d3d431e75a7c906504a079bfe07 (5000)> blackbox -l 6