Patchwork [3,of,7] transaction: add onclose hook for pre-close logic

login
register
mail settings
Submitter Durham Goode
Date March 25, 2014, 2:33 a.m.
Message ID <c84f51f8f92e0a3db488.1395714832@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/4051/
State Superseded
Headers show

Comments

Durham Goode - March 25, 2014, 2:33 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1395701867 25200
#      Mon Mar 24 15:57:47 2014 -0700
# Node ID c84f51f8f92e0a3db4888ac9739f43fd866cac20
# Parent  08595987c5b0e8af5aa8fec4debd7260f5a79e8f
transaction: add onclose hook for pre-close logic

Adds an optional onclose parameter to transactions that gets called just before
the transaction is committed. This allows things that build up data over the
course of the transaction (like the fncache) to commit their data.
David Soria Parra - March 26, 2014, 1:23 a.m.
Durham Goode <durham@fb.com> writes:

> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1395701867 25200
> #      Mon Mar 24 15:57:47 2014 -0700
> # Node ID c84f51f8f92e0a3db4888ac9739f43fd866cac20
> # Parent  08595987c5b0e8af5aa8fec4debd7260f5a79e8f
> transaction: add onclose hook for pre-close logic
>
> Adds an optional onclose parameter to transactions that gets called just before
> the transaction is committed. This allows things that build up data over the
> course of the transaction (like the fncache) to commit their data.
>
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -42,12 +42,14 @@
>      opener.unlink(journal)
>  
>  class transaction(object):
> -    def __init__(self, report, opener, journal, after=None, createmode=None):
> +    def __init__(self, report, opener, journal, after=None, createmode=None,
> +            onclose=None):
>          self.count = 1
>          self.usages = 1
>          self.report = report
>          self.opener = opener
>          self.after = after
> +        self.onclose = onclose
>          self.entries = []
>          self.map = {}
>          self.journal = journal
> @@ -126,6 +128,9 @@
>      @active
>      def close(self):
>          '''commit the transaction'''
> +        if self.count == 1 and self.onclose:
> +            self.onclose()
> +
I think we can do a

  if self.onclose:
     self.onclose()

just after

  self.count -= 1
  if self.count != 0:
    return

so together with the rest of the actions done
the last reference is closed.
Durham Goode - March 26, 2014, 1:45 a.m.
On 3/25/14 6:23 PM, "David Soria Parra" <davidsp@fb.com> wrote:

>Durham Goode <durham@fb.com> writes:
>
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1395701867 25200
>> #      Mon Mar 24 15:57:47 2014 -0700
>> # Node ID c84f51f8f92e0a3db4888ac9739f43fd866cac20
>> # Parent  08595987c5b0e8af5aa8fec4debd7260f5a79e8f
>> transaction: add onclose hook for pre-close logic
>>
>> Adds an optional onclose parameter to transactions that gets called
>>just before
>> the transaction is committed. This allows things that build up data
>>over the
>> course of the transaction (like the fncache) to commit their data.
>>
>> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
>> --- a/mercurial/transaction.py
>> +++ b/mercurial/transaction.py
>> @@ -42,12 +42,14 @@
>>      opener.unlink(journal)
>>  
>>  class transaction(object):
>> -    def __init__(self, report, opener, journal, after=None,
>>createmode=None):
>> +    def __init__(self, report, opener, journal, after=None,
>>createmode=None,
>> +            onclose=None):
>>          self.count = 1
>>          self.usages = 1
>>          self.report = report
>>          self.opener = opener
>>          self.after = after
>> +        self.onclose = onclose
>>          self.entries = []
>>          self.map = {}
>>          self.journal = journal
>> @@ -126,6 +128,9 @@
>>      @active
>>      def close(self):
>>          '''commit the transaction'''
>> +        if self.count == 1 and self.onclose:
>> +            self.onclose()
>> +
>I think we can do a
>
>  if self.onclose:
>     self.onclose()
>
>just after
>
>  self.count -= 1
>  if self.count != 0:
>    return
>
>so together with the rest of the actions done
>the last reference is closed.

I thought about that, but if onclose throws an exception, the tr.release()
won't call abort if self.count == 0.  So I need to do onclose before count
is decremented.
Gregory Szorc - March 26, 2014, 1:53 a.m.
On 3/24/14, 7:33 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1395701867 25200
> #      Mon Mar 24 15:57:47 2014 -0700
> # Node ID c84f51f8f92e0a3db4888ac9739f43fd866cac20
> # Parent  08595987c5b0e8af5aa8fec4debd7260f5a79e8f
> transaction: add onclose hook for pre-close logic
>
> Adds an optional onclose parameter to transactions that gets called just before
> the transaction is committed. This allows things that build up data over the
> course of the transaction (like the fncache) to commit their data.
>
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -42,12 +42,14 @@
>       opener.unlink(journal)
>
>   class transaction(object):
> -    def __init__(self, report, opener, journal, after=None, createmode=None):
> +    def __init__(self, report, opener, journal, after=None, createmode=None,
> +            onclose=None):
>           self.count = 1
>           self.usages = 1
>           self.report = report
>           self.opener = opener
>           self.after = after
> +        self.onclose = onclose
>           self.entries = []
>           self.map = {}
>           self.journal = journal
> @@ -126,6 +128,9 @@
>       @active
>       def close(self):
>           '''commit the transaction'''
> +        if self.count == 1 and self.onclose:
> +            self.onclose()
> +
>           self.count -= 1
>           if self.count != 0:
>               return

I don't mean to scope bloat this patch, but appending extra operations 
to transaction semantics is very appealing to me. We (Mozilla) have some 
extensions performing SQL against a local SQLite database, for example 
and would like the SQL to execute in a transaction tied to the Mercurial 
transaction.

I was wondering if a wrappable function is the best solution here. Given 
we're dealing with transactions and atomicity is hard, I don't believe 
wrapping functions is the most robust solution. For example, what 
happens when onclose() is wrapped multiple times and there is an 
exception in function 3 of 5? Will the commits of functions 1 and 2 have 
the opportunity to be rewound? (No they won't.)

I was curious if you'll consider a patch that makes things a bit more 
robust by adding a "transaction operation" API that allows custom 
objects to be registered to a transaction. They would have methods like 
"commit," "abort," and "rollback" that would be called during 
corresponding transaction events.
Durham Goode - March 26, 2014, 2:19 a.m.
On 3/25/14 6:53 PM, "Gregory Szorc" <gregory.szorc@gmail.com> wrote:

>On 3/24/14, 7:33 PM, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1395701867 25200
>> #      Mon Mar 24 15:57:47 2014 -0700
>> # Node ID c84f51f8f92e0a3db4888ac9739f43fd866cac20
>> # Parent  08595987c5b0e8af5aa8fec4debd7260f5a79e8f
>> transaction: add onclose hook for pre-close logic
>>
>> Adds an optional onclose parameter to transactions that gets called
>>just before
>> the transaction is committed. This allows things that build up data
>>over the
>> course of the transaction (like the fncache) to commit their data.
>>
>> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
>> --- a/mercurial/transaction.py
>> +++ b/mercurial/transaction.py
>> @@ -42,12 +42,14 @@
>>       opener.unlink(journal)
>>
>>   class transaction(object):
>> -    def __init__(self, report, opener, journal, after=None,
>>createmode=None):
>> +    def __init__(self, report, opener, journal, after=None,
>>createmode=None,
>> +            onclose=None):
>>           self.count = 1
>>           self.usages = 1
>>           self.report = report
>>           self.opener = opener
>>           self.after = after
>> +        self.onclose = onclose
>>           self.entries = []
>>           self.map = {}
>>           self.journal = journal
>> @@ -126,6 +128,9 @@
>>       @active
>>       def close(self):
>>           '''commit the transaction'''
>> +        if self.count == 1 and self.onclose:
>> +            self.onclose()
>> +
>>           self.count -= 1
>>           if self.count != 0:
>>               return
>
>I don't mean to scope bloat this patch, but appending extra operations
>to transaction semantics is very appealing to me. We (Mozilla) have some
>extensions performing SQL against a local SQLite database, for example
>and would like the SQL to execute in a transaction tied to the Mercurial
>transaction.
>
>I was wondering if a wrappable function is the best solution here. Given
>we're dealing with transactions and atomicity is hard, I don't believe
>wrapping functions is the most robust solution. For example, what
>happens when onclose() is wrapped multiple times and there is an
>exception in function 3 of 5? Will the commits of functions 1 and 2 have
>the opportunity to be rewound? (No they won't.)
>
>I was curious if you'll consider a patch that makes things a bit more
>robust by adding a "transaction operation" API that allows custom
>objects to be registered to a transaction. They would have methods like
>"commit," "abort," and "rollback" that would be called during
>corresponding transaction events.

As a first step, I could add an onabort handler to transactions that's
called at the beginning of an abort. That would give extensions a way to
hook in to the abort phase (so functions 1 and 2 could rollback).

I'd rather not add a full transaction extensibility framework to this
patch, mainly because of the amount of design involved. (How do you
register? Statically at uisetup time? Dynamically as the transactions are
being passed around? How do you serialize information to make hg recover
work?  Assume the extension will handle that bit?)  And my need to get
this series in by the end of the week.
Gregory Szorc - March 26, 2014, 3:02 a.m.
On 3/25/14, 7:19 PM, Durham Goode wrote:
> On 3/25/14 6:53 PM, "Gregory Szorc" <gregory.szorc@gmail.com> wrote:
>
>> On 3/24/14, 7:33 PM, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1395701867 25200
>>> #      Mon Mar 24 15:57:47 2014 -0700
>>> # Node ID c84f51f8f92e0a3db4888ac9739f43fd866cac20
>>> # Parent  08595987c5b0e8af5aa8fec4debd7260f5a79e8f
>>> transaction: add onclose hook for pre-close logic
>>>
>>> Adds an optional onclose parameter to transactions that gets called
>>> just before
>>> the transaction is committed. This allows things that build up data
>>> over the
>>> course of the transaction (like the fncache) to commit their data.
>>>
>>> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
>>> --- a/mercurial/transaction.py
>>> +++ b/mercurial/transaction.py
>>> @@ -42,12 +42,14 @@
>>>        opener.unlink(journal)
>>>
>>>    class transaction(object):
>>> -    def __init__(self, report, opener, journal, after=None,
>>> createmode=None):
>>> +    def __init__(self, report, opener, journal, after=None,
>>> createmode=None,
>>> +            onclose=None):
>>>            self.count = 1
>>>            self.usages = 1
>>>            self.report = report
>>>            self.opener = opener
>>>            self.after = after
>>> +        self.onclose = onclose
>>>            self.entries = []
>>>            self.map = {}
>>>            self.journal = journal
>>> @@ -126,6 +128,9 @@
>>>        @active
>>>        def close(self):
>>>            '''commit the transaction'''
>>> +        if self.count == 1 and self.onclose:
>>> +            self.onclose()
>>> +
>>>            self.count -= 1
>>>            if self.count != 0:
>>>                return
>>
>> I don't mean to scope bloat this patch, but appending extra operations
>> to transaction semantics is very appealing to me. We (Mozilla) have some
>> extensions performing SQL against a local SQLite database, for example
>> and would like the SQL to execute in a transaction tied to the Mercurial
>> transaction.
>>
>> I was wondering if a wrappable function is the best solution here. Given
>> we're dealing with transactions and atomicity is hard, I don't believe
>> wrapping functions is the most robust solution. For example, what
>> happens when onclose() is wrapped multiple times and there is an
>> exception in function 3 of 5? Will the commits of functions 1 and 2 have
>> the opportunity to be rewound? (No they won't.)
>>
>> I was curious if you'll consider a patch that makes things a bit more
>> robust by adding a "transaction operation" API that allows custom
>> objects to be registered to a transaction. They would have methods like
>> "commit," "abort," and "rollback" that would be called during
>> corresponding transaction events.
>
> As a first step, I could add an onabort handler to transactions that's
> called at the beginning of an abort. That would give extensions a way to
> hook in to the abort phase (so functions 1 and 2 could rollback).
>
> I'd rather not add a full transaction extensibility framework to this
> patch, mainly because of the amount of design involved. (How do you
> register? Statically at uisetup time? Dynamically as the transactions are
> being passed around? How do you serialize information to make hg recover
> work?  Assume the extension will handle that bit?)  And my need to get
> this series in by the end of the week.

I think onabort would suffice for the near term. A robust solution can 
come later, if it's needed.

Patch

diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -42,12 +42,14 @@ 
     opener.unlink(journal)
 
 class transaction(object):
-    def __init__(self, report, opener, journal, after=None, createmode=None):
+    def __init__(self, report, opener, journal, after=None, createmode=None,
+            onclose=None):
         self.count = 1
         self.usages = 1
         self.report = report
         self.opener = opener
         self.after = after
+        self.onclose = onclose
         self.entries = []
         self.map = {}
         self.journal = journal
@@ -126,6 +128,9 @@ 
     @active
     def close(self):
         '''commit the transaction'''
+        if self.count == 1 and self.onclose:
+            self.onclose()
+
         self.count -= 1
         if self.count != 0:
             return