Patchwork [V3] transaction: allow running file generators after finalizers

login
register
mail settings
Submitter Durham Goode
Date April 7, 2016, 9:11 p.m.
Message ID <5e49537579979f112688.1460063501@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/14424/
State Accepted
Headers show

Comments

Durham Goode - April 7, 2016, 9:11 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1460063449 25200
#      Thu Apr 07 14:10:49 2016 -0700
# Node ID 5e49537579979f11268847526f83bb9dc2baf8d8
# Parent  ea86cdcd9b50bf38c6b9dd7bbaa04b9c8cc0aefb
transaction: allow running file generators after finalizers

Previously, transaction.close would run the file generators before running the
finalizers (see the list below for what is in each). Since file generators
contain the bookmarks and the dirstate, this meant we made the dirstate and
bookmarks visible to external readers before we actually wrote the commits into
the changelog, which could result in missing bookmarks and missing working copy
parents (especially on servers with high commit throughput, since pulls might
fail to see certain bookmarks in this situation).

By moving the changelog writing to be before the bookmark/dirstate writing, we
ensure the commits are present before they are referenced.

This implementation allows certain file generators to be after the finalizers.
We didn't want to move all of the generators, since it's important that things
like phases actually run before the finalizers (otherwise you could expose
commits as public when they really shouldn't be).

For reference, file generators currently consist of: bookmarks, dirstate, and
phases. Finalizers currently consist of: changelog, revbranchcache, and fncache.
Augie Fackler - April 9, 2016, 1:52 a.m.
On Thu, Apr 07, 2016 at 02:11:41PM -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1460063449 25200
> #      Thu Apr 07 14:10:49 2016 -0700
> # Node ID 5e49537579979f11268847526f83bb9dc2baf8d8
> # Parent  ea86cdcd9b50bf38c6b9dd7bbaa04b9c8cc0aefb
> transaction: allow running file generators after finalizers
>

queued, thanks

> Previously, transaction.close would run the file generators before running the
> finalizers (see the list below for what is in each). Since file generators
> contain the bookmarks and the dirstate, this meant we made the dirstate and
> bookmarks visible to external readers before we actually wrote the commits into
> the changelog, which could result in missing bookmarks and missing working copy
> parents (especially on servers with high commit throughput, since pulls might
> fail to see certain bookmarks in this situation).
>
> By moving the changelog writing to be before the bookmark/dirstate writing, we
> ensure the commits are present before they are referenced.
>
> This implementation allows certain file generators to be after the finalizers.
> We didn't want to move all of the generators, since it's important that things
> like phases actually run before the finalizers (otherwise you could expose
> commits as public when they really shouldn't be).
>
> For reference, file generators currently consist of: bookmarks, dirstate, and
> phases. Finalizers currently consist of: changelog, revbranchcache, and fncache.
>
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -23,6 +23,19 @@ from . import (
>
>  version = 2
>
> +# These are the file generators that should only be executed after the
> +# finalizers are done, since they rely on the output of the finalizers (like
> +# the changelog having been written).
> +postfinalizegenerators = set([
> +    'bookmarks',
> +    'dirstate'
> +])
> +
> +class GenerationGroup(object):
> +    ALL='all'
> +    PREFINALIZE='prefinalize'
> +    POSTFINALIZE='postfinalize'
> +
>  def active(func):
>      def _active(self, *args, **kwds):
>          if self.count == 0:
> @@ -276,12 +289,19 @@ class transaction(object):
>          # but for bookmarks that are handled outside this mechanism.
>          self._filegenerators[genid] = (order, filenames, genfunc, location)
>
> -    def _generatefiles(self, suffix=''):
> +    def _generatefiles(self, suffix='', group=GenerationGroup.ALL):
>          # write files registered for generation
>          any = False
> -        for entry in sorted(self._filegenerators.values()):
> +        for id, entry in sorted(self._filegenerators.iteritems()):
>              any = True
>              order, filenames, genfunc, location = entry
> +
> +            # for generation at closing, check if it's before or after finalize
> +            postfinalize = group == GenerationGroup.POSTFINALIZE
> +            if (group != GenerationGroup.ALL and
> +                (id in postfinalizegenerators) != (postfinalize)):
> +                continue
> +
>              vfs = self._vfsmap[location]
>              files = []
>              try:
> @@ -407,10 +427,11 @@ class transaction(object):
>          '''commit the transaction'''
>          if self.count == 1:
>              self.validator(self)  # will raise exception if needed
> -            self._generatefiles()
> +            self._generatefiles(group=GenerationGroup.PREFINALIZE)
>              categories = sorted(self._finalizecallback)
>              for cat in categories:
>                  self._finalizecallback[cat](self)
> +            self._generatefiles(group=GenerationGroup.POSTFINALIZE)
>
>          self.count -= 1
>          if self.count != 0:
> diff --git a/tests/test-bookmarks.t b/tests/test-bookmarks.t
> --- a/tests/test-bookmarks.t
> +++ b/tests/test-bookmarks.t
> @@ -846,3 +846,52 @@ updates the working directory and curren
>    6:81dcce76aa0b
>    $ hg -R ../cloned-bookmarks-update bookmarks | grep ' Y '
>     * Y                         6:81dcce76aa0b
> +
> +  $ cd ..
> +
> +ensure changelog is written before bookmarks
> +  $ hg init orderrepo
> +  $ cd orderrepo
> +  $ touch a
> +  $ hg commit -Aqm one
> +  $ hg book mybook
> +  $ echo a > a
> +
> +  $ cat > $TESTTMP/pausefinalize.py <<EOF
> +  > from mercurial import extensions, localrepo
> +  > import os, time
> +  > def transaction(orig, self, desc, report=None):
> +  >    tr = orig(self, desc, report)
> +  >    def sleep(*args, **kwargs):
> +  >        retry = 20
> +  >        while retry > 0 and not os.path.exists("$TESTTMP/unpause"):
> +  >            retry -= 1
> +  >            time.sleep(0.5)
> +  >        if os.path.exists("$TESTTMP/unpause"):
> +  >            os.remove("$TESTTMP/unpause")
> +  >    # It is important that this finalizer start with 'a', so it runs before
> +  >    # the changelog finalizer appends to the changelog.
> +  >    tr.addfinalize('a-sleep', sleep)
> +  >    return tr
> +  >
> +  > def extsetup(ui):
> +  >    # This extension inserts an artifical pause during the transaction
> +  >    # finalizer, so we can run commands mid-transaction-close.
> +  >    extensions.wrapfunction(localrepo.localrepository, 'transaction',
> +  >                            transaction)
> +  > EOF
> +  $ hg commit -qm two --config extensions.pausefinalize=$TESTTMP/pausefinalize.py &
> +  $ sleep 2
> +  $ hg log -r .
> +  changeset:   0:867bc5792c8c
> +  bookmark:    mybook
> +  tag:         tip
> +  user:        test
> +  date:        Thu Jan 01 00:00:00 1970 +0000
> +  summary:     one
> +
> +  $ hg bookmarks
> +   * mybook                    0:867bc5792c8c
> +  $ touch $TESTTMP/unpause
> +
> +  $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -23,6 +23,19 @@  from . import (
 
 version = 2
 
+# These are the file generators that should only be executed after the
+# finalizers are done, since they rely on the output of the finalizers (like
+# the changelog having been written).
+postfinalizegenerators = set([
+    'bookmarks',
+    'dirstate'
+])
+
+class GenerationGroup(object):
+    ALL='all'
+    PREFINALIZE='prefinalize'
+    POSTFINALIZE='postfinalize'
+
 def active(func):
     def _active(self, *args, **kwds):
         if self.count == 0:
@@ -276,12 +289,19 @@  class transaction(object):
         # but for bookmarks that are handled outside this mechanism.
         self._filegenerators[genid] = (order, filenames, genfunc, location)
 
-    def _generatefiles(self, suffix=''):
+    def _generatefiles(self, suffix='', group=GenerationGroup.ALL):
         # write files registered for generation
         any = False
-        for entry in sorted(self._filegenerators.values()):
+        for id, entry in sorted(self._filegenerators.iteritems()):
             any = True
             order, filenames, genfunc, location = entry
+
+            # for generation at closing, check if it's before or after finalize
+            postfinalize = group == GenerationGroup.POSTFINALIZE
+            if (group != GenerationGroup.ALL and
+                (id in postfinalizegenerators) != (postfinalize)):
+                continue
+
             vfs = self._vfsmap[location]
             files = []
             try:
@@ -407,10 +427,11 @@  class transaction(object):
         '''commit the transaction'''
         if self.count == 1:
             self.validator(self)  # will raise exception if needed
-            self._generatefiles()
+            self._generatefiles(group=GenerationGroup.PREFINALIZE)
             categories = sorted(self._finalizecallback)
             for cat in categories:
                 self._finalizecallback[cat](self)
+            self._generatefiles(group=GenerationGroup.POSTFINALIZE)
 
         self.count -= 1
         if self.count != 0:
diff --git a/tests/test-bookmarks.t b/tests/test-bookmarks.t
--- a/tests/test-bookmarks.t
+++ b/tests/test-bookmarks.t
@@ -846,3 +846,52 @@  updates the working directory and curren
   6:81dcce76aa0b
   $ hg -R ../cloned-bookmarks-update bookmarks | grep ' Y '
    * Y                         6:81dcce76aa0b
+
+  $ cd ..
+
+ensure changelog is written before bookmarks
+  $ hg init orderrepo
+  $ cd orderrepo
+  $ touch a
+  $ hg commit -Aqm one
+  $ hg book mybook
+  $ echo a > a
+
+  $ cat > $TESTTMP/pausefinalize.py <<EOF
+  > from mercurial import extensions, localrepo
+  > import os, time
+  > def transaction(orig, self, desc, report=None):
+  >    tr = orig(self, desc, report)
+  >    def sleep(*args, **kwargs):
+  >        retry = 20
+  >        while retry > 0 and not os.path.exists("$TESTTMP/unpause"):
+  >            retry -= 1
+  >            time.sleep(0.5)
+  >        if os.path.exists("$TESTTMP/unpause"):
+  >            os.remove("$TESTTMP/unpause")
+  >    # It is important that this finalizer start with 'a', so it runs before
+  >    # the changelog finalizer appends to the changelog.
+  >    tr.addfinalize('a-sleep', sleep)
+  >    return tr
+  > 
+  > def extsetup(ui):
+  >    # This extension inserts an artifical pause during the transaction
+  >    # finalizer, so we can run commands mid-transaction-close.
+  >    extensions.wrapfunction(localrepo.localrepository, 'transaction',
+  >                            transaction)
+  > EOF
+  $ hg commit -qm two --config extensions.pausefinalize=$TESTTMP/pausefinalize.py &
+  $ sleep 2
+  $ hg log -r .
+  changeset:   0:867bc5792c8c
+  bookmark:    mybook
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     one
+  
+  $ hg bookmarks
+   * mybook                    0:867bc5792c8c
+  $ touch $TESTTMP/unpause
+
+  $ cd ..