New transaction-scheme
sho...@email.de
first of all, I wanna apologize for not contributing for 2 months. Some of you may remember, that in the course of introducing wiki-page renames, which I wanted to revive, the issue of transaction management came up, which currently is done with handle_ta flags. Remy made a suggestion to implement a decorator as_transaction, and I prematurely said I would implement that.
Well, I tried, but I have had not too much python experience, and never made any more complicated decorators before, and so had some problems, primarily with scoping.
So I set it aside for the moment being, and then a lot of other stuff came up for me. Anyway, now I decided to pick it up again, and I have the decorator implemented now. which is attached in a diff, which also contains the basic implementation and testing of the rename functionality in the wiki model, although without dealing with attachments yet (this is just to demonstrate the decorator).
So, the decorator works fine now, but still has a leftover from the problems I ran into 2 months ago which drives me crazy:
def as_transaction(env, db):
"""
Decorator for transactions.
Usage:
-- def api_method(p1, p2):
-- @as_transaction(env, db)
-- def implementation_method(p3, p4, db):
-- # implementation
--
-- implementation_method()
"""
def transaction_creator(fn):
db0 = db
def transaction(*args, **kwargs):
db = db0
if not db:
db = env.get_db_cnx()
try:
fn(db = db, *args, **kwargs)
db.commit()
except Exception, e:
db.rollback()
else:
fn(db = db, *args, **kwargs)
return transaction
return transaction_creator
This db0 = db line. For some reason, I just couldn't figure out, over the course of the defining of the sub-functions, db becomes undefined, but env does not. The fix is to define a second variable db0 in the middle, that doesn't get out of scope (or whatever happens there). I really would like to understand what happens there, and then get rid of this ugly workaround. Did I stumble over an error in python?
So, I have more time again now, and when this as_transaction thing is accepted, I would like to pick up the rename again.
Regards
Jan Schukat
Christian Boos
sho...@email.de wrote:
> Hello,
>
> ...
>
> So, the decorator works fine now, but still has a leftover from the problems I ran into 2 months ago which drives me crazy:
>
> def as_transaction(env, db):
> """
> Decorator for transactions.
> Usage:
> -- def api_method(p1, p2):
> -- @as_transaction(env, db)
> -- def implementation_method(p3, p4, db):
> -- # implementation
> --
> -- implementation_method()
> """
> def transaction_creator(fn):
> db0 = db
> def transaction(*args, **kwargs):
> db = db0
> if not db:
> db = env.get_db_cnx()
> ...
> This db0 = db line. For some reason, I just couldn't figure out, over the course of the defining of the sub-functions, db becomes undefined, but env does not.
The fact that you assign to the db variable in your local function makes
it a local variable. It doesn't matter if that assignment is executed or
not, it's just a "compile" time decision, in the same way as a function
becomes a generator if there's a yield inside.
Other than that, yes, it seems worth to switch to something like that,
I'll test the patch.
-- Christian
sho...@email.de
Regard
Jan Schukat
Christian Boos
> Then why does this not happen to the env parameter? This is what drives me crazy: the env is available troughout, the db is not, despite both being exactly the same in their usage.
>
Please re-read what I wrote, it's because of the assignment `db =
env.get_db_cnx()`. As you don't assign to env, it doesn't become a
variable local to the "transaction" local function but is obtained from
the outer scope.
See http://docs.python.org/reference/executionmodel.html#naming
"If a name binding operation occurs anywhere within a code block, all
uses of the name within the block are treated as references to the
current block ... "
-- Christian
PS: your MUA has some mail threading issues ...
Remy Blank
> Usage:
> -- def api_method(p1, p2):
> -- @as_transaction(env, db)
> -- def implementation_method(p3, p4, db):
> -- # implementation
> --
> -- implementation_method()
Why not run transaction() in the decorator directly in
transaction_creator, instead of returning it? Or rather, drop
transaction() and execute the code in transaction_creator(). This would
allow writing the following:
def api_method(p1, p2):
@as_transaction(env, db)
def implementation_method(db, p3, p4):
# implementation
Maybe name the decorator "run_as_transaction" to make it clear that it
actually runs the decorated function.
-- Remy
Christian Boos
> sho...@email.de wrote:
>
>> Usage:
>> -- def api_method(p1, p2):
>> -- @as_transaction(env, db)
>> -- def implementation_method(p3, p4, db):
>> -- # implementation
>> --
>> -- implementation_method()
>>
>
> Why not run transaction() in the decorator directly in
> transaction_creator, instead of returning it? Or rather, drop
> transaction() and execute the code in transaction_creator(). This would
> allow writing the following:
>
> def api_method(p1, p2):
> @as_transaction(env, db)
> def implementation_method(db, p3, p4):
> # implementation
>
Ha, I had *exactly* the same thought... but then I tried to implement
it, and it doesn't seem to be possible, as you must return a callable
which takes the to-be-decorated function as input, and that callable
won't get called until the decorated function is called...
But I'd be pleased to be proved wrong, as this would make things nicer.
-- Christian
Remy Blank
> Ha, I had *exactly* the same thought... but then I tried to implement
> it, and it doesn't seem to be possible, as you must return a callable
> which takes the to-be-decorated function as input, and that callable
> won't get called until the decorated function is called...
You can call the to-be-decorated function in the callable returned by
as_transaction, can't you?
def run_as_transaction(env, db):
def decorator(fn):
if not db:
db = env.get_db_cnx()
try:
fn(db)
except Exception:
db.rollback()
else:
db.commit()
else:
fn(db)
return decorator
def api_method(self):
@run_as_transaction(env, db)
def change_something(db):
cursor = db.cursor()
cursor.execute("UPDATE ...")
This will leave a local named "change_something" with the value None.
Not tested, of course...
-- Remy
sho...@email.de
Also, I wouldn't know how to pass arguments along then.
Jan
sho...@email.de
Jan
Remy Blank
> I have a working implementation with the direct call now, but as I
> have said, it can't pass arguments along.
That doesn't really matter, as we have access to the outer scope. If you
really wanted to pass arguments, you could pass them to the decorator
instead of the function. But I don't see a use case for that.
-- Remy
sho...@email.de
Jan
Christian Boos
> Ok, here the diff.
>
> Have both decorator versions in it now, but use the direct run_as_transaction decorator in the tests.
>
>
Ok, thank you and Remy for the heads up, I think I must have missed the
double nesting, so ... right it seems this can be done after all,
perfect :P)
I have a couple of follow-up suggestions:
- We could rename @run_as_transaction to @with_transaction, suggesting
the evolution to using 'with' when moving to 2.5, where this will become
"with transaction(env, db) as db: ..."
- I think we don't need fn(db=...) here, as Remy said we can always
get use anything from the outer scope, so no need to pass any other
arguments besides db
- We could return True/False/None depending on the outcome: True when
committed, False when rollbacked, None if still within a transaction
So this results into something like:
def with_transaction(env, db):
""" ... """
def wrap(fn):
if db:
fn(db)
else:
tmpdb = env.get_db_cnx()
try:
fn(tmpdb)
tmpdb.commit()
return True
except Exception, e:
tmpdb.rollback()
return False
return wrap
-- Christian
sho...@email.de
Jan
Remy Blank
> - We could rename @run_as_transaction to @with_transaction, suggesting
> the evolution to using 'with' when moving to 2.5, where this will become
> "with transaction(env, db) as db: ..."
I like that. +1
> - We could return True/False/None depending on the outcome: True when
> committed, False when rollbacked, None if still within a transaction
I don't think there will be an equivalent mechanism when using the
"with" statement, so I'm not sure it's a good idea to introduce that.
-- Remy
Christian Boos
In-Reply-To header... it's getting annoying to see a new thread each
time you reply...)
sho...@email.de wrote:
> How would one check those return values? I imagine that would be very awkward syntax.
>
It will get assigned to the name used for the implementation method, e.g.
@with_transaction(env, db)
def rename(db):
# ... transaction body ...
if rename is False:
print "rename failed"
# perform some related clean-up
I imagined this could be handy in some cases, but actually this will
have no direct equivalent when transitioning to the "with" syntax, so I
think we should in fact not do this.
Rather, the comparison with the "with" approach made me think we should
instead propagate the value returned by the implementation function,
otherwise we would have no way to communicate something from there to
the outside. When using "with", we will have no problem modifying local
variables from the calling context, something we can't do with the
decorator approach, for the same reasons as discussed in previous mail
regarding the "strange behavior" of db.
Also, this made me experiment with what we should do with exceptions
occurring in the transaction body (either propagate them or trap them).
We have 3 use cases:
1. we want to ignore any exception
2. we want to do something when a specific exception is raised in the
body of the transaction
3. we want the exception to be re-raised
If we always trap the exception, we would get 1. and for 2. we could add
a try/except block within the transaction body. But then we couldn't get 3.
If we always re-raise the exception, then we get 3. by default, and for
1. or 2. we would have to add a try/except.
So maybe the best approach would be a flag passed to the decorator (or
context manager) telling if the exception should be propagated, so we
could have 1. by default and not have to write a try/except in this
case, 2. by writing an exception handler inside the transaction body and
3. by setting that propagate flag to True.
See self-contained experiment, attached.
In this with_transaction.py file, there are 4 "do_rename_..." functions,
a pair demonstrating the decorator approach, with and without
propagating exceptions raised in the transaction body, and another pair
doing the same for the context manager approach. As one can see, both
approach are quite clean, the "with" syntax being the cleanest and less
verbose.
With something like that, we have a simple transition path to using
"with transaction() as db" in 0.13.
-- Christian
Christian Boos
I agree, but see the "sibling" thread, where I develop that further...
-- Christian
Jan Schukat
Erik Bray
>
> sho...@email.de wrote:
>> Ok, here the diff.
>>
>> Have both decorator versions in it now, but use the direct run_as_transaction decorator in the tests.
>>
>>
>
> Ok, thank you and Remy for the heads up, I think I must have missed the
> double nesting, so ... right it seems this can be done after all,
> perfect :P)
>
> I have a couple of follow-up suggestions:
> - We could rename @run_as_transaction to @with_transaction, suggesting
> the evolution to using 'with' when moving to 2.5, where this will become
> "with transaction(env, db) as db: ..."
> - I think we don't need fn(db=...) here, as Remy said we can always
> get use anything from the outer scope, so no need to pass any other
> arguments besides db
> - We could return True/False/None depending on the outcome: True when
> committed, False when rollbacked, None if still within a transaction
So is that like
enum bool {
TRUE,
FALSE,
FILE_NOT_FOUND
}
?
(Sorry, sorry, I just couldn't resist. If you're a reader of
dailywtf.com it's a running joke there.)
Remy Blank
> Rather, the comparison with the "with" approach made me think we should
> instead propagate the value returned by the implementation function,
> otherwise we would have no way to communicate something from there to
> the outside.
Not quite true. The usual trick is to allocate a list in the surrounding
scope, and to mutate the array in the transaction function.
retval = [0]
@with_transaction(env, db)
def renamne(db):
# ... transaction body ...
retval[0] = 1
> So maybe the best approach would be a flag passed to the decorator (or
> context manager) telling if the exception should be propagated, so we
> could have 1. by default and not have to write a try/except in this
> case, 2. by writing an exception handler inside the transaction body and
> 3. by setting that propagate flag to True.
I don't like that all too much, but maybe it's just because I don't have
a use case yet where we would like to catch the exception. I think I
would rather have an explicit try: except: block if I want to catch
exceptions, as opposed to its being "swallowed" magically by the
decorator or context manager. Remember, explicit is better than implicit :)
Could we start with as little magic as possible, and only add the magic
if/when we really need it?
-- Remy
Christian Boos
> Christian Boos wrote:
>
>> Rather, the comparison with the "with" approach made me think we should
>> instead propagate the value returned by the implementation function,
>> otherwise we would have no way to communicate something from there to
>> the outside.
>>
>
> Not quite true. The usual trick is to allocate a list in the surrounding
> scope, and to mutate the array in the transaction function.
>
> retval = [0]
>
> @with_transaction(env, db)
> def renamne(db):
> # ... transaction body ...
> retval[0] = 1
>
Ah yes ;-)
That's more explicit than the return trick, and when switching to
"with", we'll just have to remove the list.
> (snip)
> Could we start with as little magic as possible, and only add the magic
> if/when we really need it?
>
Ok, reading more about context managers show that swallowing exceptions
should be ... the exception.
Updated sample code accordingly.
-- Christian
Remy Blank
> Updated sample code accordingly.
Looks very good to me. And considering that in most cases, we won't have
to return values from the transaction, the decorator solution is very
readable.
-- Remy
Jan Schukat
I have installed a python2.4 again, and created two implementations of
the transaction scheme, one for 2.4, which uses decorators, and one
for 2.5, which uses with statements. In both cases I converted the
WikiPage class to use the scheme, and both pass the complete unit test
and functional test suits in the python versions they are supposed to
work in.
Both are very very similar in their use, but I do think now, it would
be unwise to have both implementations present at the same time in the
code. Whether both implementations are present or not, it wold need
need cleanup, once 2.4 support is dropped, and in the case of just
having to replace the decorators with with-statements and the context
managers, without all that version check code, this could be done with
a quite simple sed script ...
What I did notice though is, that you can't get variables out of the
with block. The overwriting of the new_name variable in the Trac 0.13
example by Christian below isn't visible outside the with block. At
least that is what the unit tests told me when I tried it. Maybe I'm
not aware of something here.
If this proposal gets accepted, I would start converting all code to
use the new scheme, as I have stated before.
If there are still objections, I would like to hear them.
Regards
Jan Schukat
Remy Blank
> If this proposal gets accepted, I would start converting all code to
> use the new scheme, as I have stated before.
I wouldn't convert all database writes to the new scheme in one big
patch. Instead, I would rather introduce it with new code (IIRC, you
wanted to work on wiki page renames) and get familiar with it, maybe
refine the scheme somewhat with the experience gained, and only then,
transition old code progressively.
Converting all writes at the same time sounds like a good way to
introduce subtle bugs all over the code. Instead, if the new
functionality is initially confined to a small section of code, the
potential damage should be confined as well.
-- Remy
Jan Schukat
definitions to the trunk, since they don't interfere with anything,
maybe without the WikiPage adjustments in delete and save. And then
the new db functionality, like my rename stuff, can start using it, so
we can get experience.
Attached is a diff, that just contains the decorators.
So either apply this patch, or the one in the mail before with the
changed WikiPage. Either way, some conclusion would be nice, so it's
clear how to proceed.
Regards
Jan
Christian Boos
Thanks for your continued efforts, we're getting there ... but not yet.
First, some general remarks. Apparently your mail program and my
Thunderbird are not friendly to each other ;-)
The first patch (transactions_py2.5.diff) was completely garbled. From
the web interface however, the patch can be obtained correctly
(http://groups.google.com/group/trac-dev/msg/dfbfd93adaabfb06?hl=en).
So, in doubt, let's blame Thunderbird here, but I would be interested to
know how it worked for others.
The second patch (transactions_py2.4.diff) contained the changes
*twice*. You must have done svn diff >> .diff at some point...
Ok, those were uninteresting comments about the form of the patches,
sorry but this is just getting in the way of being able to review
comfortably the changes.
Now, slightly more on topic, you should really try to conform to the
coding style adopted by the Trac project, as those are prerequisites for
any changes (remember that I already made such comments in #1106 - (1)).
In particular, we try to conform to the standard Python coding style
(PEP 8 (2)), and there are constructs in your changes that are quite at
odds with those guidelines (more than one statement per line, if(test),
...).
Jan Schukat wrote:
> Hello again,
>
> I have installed a python2.4 again, and created two implementations of
> the transaction scheme, one for 2.4, which uses decorators, and one
> for 2.5, which uses with statements. In both cases I converted the
> WikiPage class to use the scheme, and both pass the complete unit test
> and functional test suits in the python versions they are supposed to
> work in.
>
Good, that's indeed *the* recommended way to validate the changes.
Writing additional tests for newly introduced features is also useful.
> Both are very very similar in their use, but I do think now, it would
> be unwise to have both implementations present at the same time in the
> code. Whether both implementations are present or not, it wold need
> need cleanup, once 2.4 support is dropped, and in the case of just
> having to replace the decorators with with-statements and the context
> managers, without all that version check code, this could be done with
> a quite simple sed script ...
>
Right, we only need the decorator based approach for now. Doing the
context manager based approach during the experimental stage is
nevertheless important so that we can be confident we're on the right
track and that the migration will be painless.
> What I did notice though is, that you can't get variables out of the
> with block. The overwriting of the new_name variable in the Trac 0.13
> example by Christian below isn't visible outside the with block. At
> least that is what the unit tests told me when I tried it. Maybe I'm
> not aware of something here.
>
Well, it should work. What do you see when you run my test program (the
one from (3))?
$ /C/Dev/Python254/python with_transaction.py
...
== (fail=False, do_rename=<function do_rename_with at 0x027E6970>)
-- testing automatic transaction
>> Environment.get_db_cnx
>> execute something successfully...
>> commit transaction
> it worked: renamed
-- testing explicit transaction
>> Environment.get_db_cnx
>> execute something successfully...
> it worked: renamed
>> execute something successfully...
> it worked: renamed
>> commit transaction
...
The "it worked: renamed" part shows that the assignment in the with
block modified the reference from the containing block:
# Trac 0.13
def do_rename_with(env, db=None):
new_name = None
with transaction(env, db) as db:
db.execute()
new_name = 'renamed' # modifies `new_name` above
if new_name:
print '> it worked:', new_name
return True
This should work, therefore we should also use the modification "by
reference" suggested by Remy for the decorator approach, modifying the
single element of an array, rather than my clumsy way of returning a
value via the function name ;-)
Btw, please take all of this as /constructive/ criticisms, I wouldn't
bother making them if I wasn't interested in seeing your contributions
integrated in the end ;-)
Now heading to your next patch...
-- Christian
> If this proposal gets accepted, I would start converting all code to
> use the new scheme, as I have stated before.
> If there are still objections, I would like to hear them.
>
(1) - http://trac.edgewall.org/ticket/1106#comment:110
(2) - http://www.python.org/dev/peps/pep-0008/
(3) - http://groups.google.com/group/trac-dev/msg/7ec20d1f259aa046
Christian Boos
Jan Schukat wrote:
> Ok, so best would probably be, to include add the decorator
> definitions to the trunk, since they don't interfere with anything,
> maybe without the WikiPage adjustments in delete and save. And then
> the new db functionality, like my rename stuff, can start using it, so
> we can get experience.
>
> Attached is a diff, that just contains the decorators.
>
This time the patch was fine for Thunderbird ;-)
Some additional comments I could have made for the previous patch as well:
- from the rest of discussion, you should have seen that Remy and me
agreed on a version without the "raise_exceptions" approach, so you
shouldn't have reintroduced that, at least not without argumentation;
same thing for the "return" part, as I already commented
- you introduced two very similar decorators, with_transaction and
with_db_transaction; the only benefit of the with_transaction one is to
get a cursor from the db, which is something very trivial to do anyway
(db.cursor()), so I don't think this warrants the extra complexity of
having *2* decorators; with_db_transaction is enough and should be
renamed with_transaction.
> So either apply this patch, or the one in the mail before with the
> changed WikiPage. Either way, some conclusion would be nice, so it's
> clear how to proceed.
>
>
Once we find the patch satisfying (i.e. all the feedback taken into
account), we'll integrate it in trunk or perhaps first in a tmp branch,
as we see fit.
(Remy wrote)
>> I wouldn't convert all database writes to the new scheme in one big
>> patch.
Agreed.
>> Instead, I would rather introduce it with new code (IIRC, you
>> wanted to work on wiki page renames) and get familiar with it, maybe
>> refine the scheme somewhat with the experience gained, and only then,
>> transition old code progressively.
>>
>>
Or, as there were also a bunch of #1106 specific issues to address
first, do it for one module, exactly like you did in your
transactions_py2.4.diff patch.
>> Converting all writes at the same time sounds like a good way to
>> introduce subtle bugs all over the code. Instead, if the new
>> functionality is initially confined to a small section of code, the
>> potential damage should be confined as well.
>>
>>
Also agreed. But we might want to make all these changes on a branch, in
order to get an overview of all the changes implied.
In particular, I'm wondering how this will interact with #6348
(http://trac.edgewall.org/ticket/6348), which I should really get around
to ...
-- Christian
Jan Schukat
I'm using the OS X Mail.app now, don't think that is unable to handle
mailing lists. I guess my mail provider is more at fault here and
messes up MessageIDs. But that would be strange, since I used that
same mail provider for apache-dev mailing lists, libstdc++-dev mailing
lists and mozilla-dev mailing lists, and no one complained.
Maybe I should dig out my gmail account, which I mainly use for spam
catching now.
2. Me messing up
Don't worry about how I might feel. When I'm wrong, I'm wrong, when I
don't know something or missed something, then I don't know or missed
something. No big deal. Only made very minor one line patch
contributions to open source projects up to now though, so this is a
little new to me, culture wise, as I mainly worked on internal
projects for big corporations so far. Also, I lack experience in
python, as you can tell, used it only for quick and dirty stuff up to
now.
Ok, enough excuses.
3. Actual issues
- have read the python coding style now
- only one decorator, that passes the db. Ok, wasn't sure about that
anyway.
- leave out raise_exceptions, always propagate
- don't like the reference approach to pass values out, since it is
sort of a hack, but it is structurally the same to what will be used
later in the with statement, it's best I guess, will not be that
common anyway
- new patch with the adjustments and the WikiPage using the
decorators will come shortly
4. Your test script
- works fine, don't know what went wrong in my tests within the trac
code
- will look into that a little more
Now I got to run some errands
Jan
Jan Schukat
Also did some tests concerning the with-block variable scoping, got no
problems this time, so I guess my problems earlier, while I was
testing different approaches, were something entirely different.
Christian Boos
> Ok, a new patch, just one decorator, WikiPage implemented.
>
Hey, this one looks great!
Remy, OK for an experimental branch, starting with this change?
-- Christian
Remy Blank
> Hey, this one looks great!
I have got some minor nitpicks:
- Use WikiFormatting for docstrings.
- The "try: except: raise" in the case where db is not None is not
necessary, just call fn(db).
- Sort imports alphabetically.
- Use "cursor" for cursors. That's the convention for the rest of Trac.
- There's still an "if(new_text)" in save().
> Remy, OK for an experimental branch, starting with this change?
A branch sounds a bit overkill for this, I would have opened a ticket
and tracked progress with patches, and integrated the result into trunk.
But I'm ok if you think it is worth the overhead.
-- Remy
sho...@email.de
Except, what is the issue with the new_text variable? That's my way of moving validity checks out of the transaction, since they don't belong there and should be done before the transaction is started. I could make the checks multiple times though, first to rule out the error case, and then to distinguish between the two update types. Just makes the code longer and less clear IMHO.
Regards
Jan
Remy Blank
> Except, what is the issue with the new_text variable? That's
> my way of moving validity checks out of the transaction, since
> they don't belong there and should be done before the transaction
> is started. I could make the checks multiple times though, first
> to rule out the error case, and then to distinguish between the
> two update types. Just makes the code longer and less clear IMHO.
I should have been more explicit, sorry. I was referring to:
if(new_text):
This looks so much like C code ;-) Use this instead:
if new_text:
-- Remy
Christian Boos
> Ok, here the updated diff, with those little things taken care of.
>
>
Thanks!
I've now created http://trac.edgewall.org/ticket/8751 for tracking
progresses on the sandbox/8751-with_transaction branch, which will be
used to generalize this approach to the rest of Trac.
Once the whole code base is converted and if in the meantime the
approach is confirmed to be sound, we'll merge it back to trunk.
-- Christian