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
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
Regard
Jan Schukat
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 ...
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
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
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
Jan
Jan
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
Jan
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
Jan
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
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
I agree, but see the "sibling" thread, where I develop that further...
-- Christian
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.)
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
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
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
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
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
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
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
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
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.
Hey, this one looks great!
Remy, OK for an experimental branch, starting with this change?
-- Christian
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
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
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
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