Patchwork [STABLE] bookmarks: backout the attempt to fix the delete race

login
register
mail settings
Submitter Matt Harbison
Date June 30, 2019, 5:15 a.m.
Message ID <874e3f6d19ba89c5cd88.1561871741@Envy>
Download mbox | patch
Permalink /patch/40730/
State Accepted
Headers show

Comments

Matt Harbison - June 30, 2019, 5:15 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1561864987 14400
#      Sat Jun 29 23:23:07 2019 -0400
# Branch stable
# Node ID 874e3f6d19ba89c5cd8864beba75d2c3e9d0c25c
# Parent  044045dce23aaeb3688bcff9bd79d67fa710228f
bookmarks: backout the attempt to fix the delete race

This backs out 044045dce23a because it broke a bunch of tests on Windows.
Yuya's theory is that we still rely on in-memory changelog data to be flushed
out of the transaction.
Yuya Nishihara - June 30, 2019, 6:01 a.m.
On Sun, 30 Jun 2019 01:15:41 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1561864987 14400
> #      Sat Jun 29 23:23:07 2019 -0400
> # Branch stable
> # Node ID 874e3f6d19ba89c5cd8864beba75d2c3e9d0c25c
> # Parent  044045dce23aaeb3688bcff9bd79d67fa710228f
> bookmarks: backout the attempt to fix the delete race

Queued for stable because we're close to the release, thanks.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1222,55 +1222,6 @@  class localrepository(object):
     @mixedrepostorecache(('bookmarks', 'plain'), ('bookmarks.current', 'plain'),
                          ('00changelog.i', ''))
     def _bookmarks(self):
-        # Since the multiple files involved in the transaction cannot be
-        # written atomically (with current repository format), there is a race
-        # condition here.
-        #
-        # 1) changelog content A is read
-        # 2) outside transaction update changelog to content B
-        # 3) outside transaction update bookmark file referring to content B
-        # 4) bookmarks file content is read and filtered against changelog-A
-        #
-        # When this happens, bookmarks against nodes missing from A are dropped.
-        #
-        # Having this happening during read is not great, but it become worse
-        # when this happen during write because the bookmarks to the "unknown"
-        # nodes will be dropped for good. However, writes happen within locks.
-        # This locking makes it possible to have a race free consistent read.
-        # For this purpose data read from disc before locking  are
-        # "invalidated" right after the locks are taken. This invalidations are
-        # "light", the `filecache` mechanism keep the data in memory and will
-        # reuse them if the underlying files did not changed. Not parsing the
-        # same data multiple times helps performances.
-        #
-        # Unfortunately in the case describe above, the files tracked by the
-        # bookmarks file cache might not have changed, but the in-memory
-        # content is still "wrong" because we used an older changelog content
-        # to process the on-disk data. So after locking, the changelog would be
-        # refreshed but `_bookmarks` would be preserved.
-        # Adding `00changelog.i` to the list of tracked file is not
-        # enough, because at the time we build the content for `_bookmarks` in
-        # (4), the changelog file has already diverged from the content used
-        # for loading `changelog` in (1)
-        #
-        # To prevent the issue, we force the changelog to be explicitly
-        # reloaded while computing `_bookmarks`. The data race can still happen
-        # without the lock (with a narrower window), but it would no longer go
-        # undetected during the lock time refresh.
-        #
-        # The new schedule is as follow
-        #
-        # 1) filecache logic detect that `_bookmarks` needs to be computed
-        # 2) cachestat for `bookmarks` and `changelog` are captured (for book)
-        # 3) We force `changelog` filecache to be tested
-        # 4) cachestat for `changelog` are captured (for changelog)
-        # 5) `_bookmarks` is computed and cached
-        #
-        # The step in (3) ensure we have a changelog at least as recent as the
-        # cache stat computed in (1). As a result at locking time:
-        #  * if the changelog did not changed since (1) -> we can reuse the data
-        #  * otherwise -> the bookmarks get refreshed.
-        self._refreshchangelog()
         return bookmarks.bmstore(self)
 
     def _refreshchangelog(self):
diff --git a/tests/test-bookmarks-corner-case.t b/tests/test-bookmarks-corner-case.t
--- a/tests/test-bookmarks-corner-case.t
+++ b/tests/test-bookmarks-corner-case.t
@@ -200,7 +200,6 @@  Check raced push output.
   $ cat push-output.txt
   pushing to ssh://user@dummy/bookrace-server
   searching for changes
-  remote has heads on branch 'default' that are not known locally: f26c3b5167d1
   remote: setting raced push up
   remote: adding changesets
   remote: adding manifests
@@ -220,7 +219,7 @@  Check result of the push.
   |  summary:     A1
   |
   | o  changeset:   3:f26c3b5167d1
-  | |  bookmark:    book-B
+  | |  bookmark:    book-B (false !)
   | |  user:        test
   | |  date:        Thu Jan 01 00:00:00 1970 +0000
   | |  summary:     B1
@@ -243,4 +242,4 @@  Check result of the push.
   
   $ hg -R bookrace-server book
      book-A                    4:9ce3b28c16de
-     book-B                    3:f26c3b5167d1
+     book-B                    3:f26c3b5167d1 (false !)