Plugin calls db.commit() when it shouldn't

[mal@…]

The ticket change hook calls db.commit() even though it is not in control of the transaction.

This can result in data inconsistencies when e.g. processing multiple tickets using scripts or other Trac mechanisms which expect that transactions are not committed in case the connection used to save the ticket changes is opened by the controlling script or other mechanism.

See ​http://trac-hacks.org/svn/timingandestimationplugin/branches/trac0.11/timingandestimationplugin/ticket_daemon.py for details.

[Russ Tyndall]

FYI: Trac 12 has a different transaction system, and this should be solved in the trac12 branches.

Is there a compelling need to update this in older versions of trac?

[Russ Tyndall]

[2893] Also, these commits were added b/c no one else was committing the transaction. This implies to me that perhaps these should remain, though in the intervening versions of trac 11 perhaps someone is now committing the transaction.

If you can offer a suggestion for the appropriate way to ensure this gets committed (if autocommit is off), but doesnt squash other peoples transactions, I would patching it.

[anonymous]

Replying to bobbysmith007:

FYI: Trac 12 has a different transaction system, and this should be solved in the trac12 branches.

Is there a compelling need to update this in older versions of trac?

Well, we used to run Trac 0.11 where this is a big problem if you want to use the plugin from a script that is supposed to be transaction based.

Of course, the same goes for Trac itself when used through the web: you don't want one plugin to commit a transaction if a plugin later in the chain fails to do its job.

[anonymous]

Replying to bobbysmith007:

[2893] Also, these commits were added b/c no one else was committing the transaction. This implies to me that perhaps these should remain, though in the intervening versions of trac 11 perhaps someone is now committing the transaction.

If you can offer a suggestion for the appropriate way to ensure this gets committed (if autocommit is off), but doesnt squash other peoples transactions, I would patching it.

There are two problems here (from memory, it's been a while since I looked at the plugin code):

• the plugin opens its own connection to the database - it should use the one Trac is using, if possible

• the plugin commits the transaction on its own connection; which is right, since otherwise the changes wouldn't get committed, but is wrong in the light of the currently ongoing changes applied by the other Trac parts in the current transaction on its connection

Also note that the Trac connection won't necessarily see the changes applied by the plugin connection and vice-versa (due to transaction isolation level settings on the database).

We're using this monkey patch in our script for Trac 0.11 to work around the issues:

    env = Environment(project_path)
    db = env.get_db_cnx()

    # Hack to work-around the transaction management problem in the
    # TimingAndEstimation plugin (it calls db.commit() on its own in
    # the ticket change handler !)
    db_commit = db.commit
    db.commit = lambda : None
    env.get_db_cnx = lambda : db

With the hack we assure that Trac will only ever use a single connection and by setting db.commit to a nop method we assure that we have full control over the transaction being committed.

I haven't yet looked at the Trac 0.12 of doing things, but will have to at the end of month, since that's when the booking script is run.

BTW: Thanks for the nice plugin !

[Russ Tyndall]

Your welcome !

One of the larger trac 12 changes was to essentially implement your patch consistently across trac

• ​get_db_cnx Deprecated trac 12 docs

I am very inclined to say you should just use trac 12 if you want proper connection management, as that is a much better tested code path, than anything I implement will be, and already works across pretty much any trac12 compatible plugin and with all three database backends.

• I think in trac 11 there will be issues with the stored db connection being shared across request contexts (at least I have seen stuff like that in the past if you have lingering connection references). It could probably be worked around though

[anonymous]

Replying to bobbysmith007:

Your welcome !

One of the larger trac 12 changes was to essentially implement your patch consistently across trac

• ​get_db_cnx Deprecated trac 12 docs

I am very inclined to say you should just use trac 12 if you want proper connection management, as that is a much better tested code path, than anything I implement will be, and already works across pretty much any trac12 compatible plugin and with all three database backends.

Well, we have moved to Trac 0.12, so we'll need to have a look at how things are done there.

For Trac 0.11, you could put up a warning on the plugin wiki page.

• I think in trac 11 there will be issues with the stored db connection being shared across request contexts (at least I have seen stuff like that in the past if you have lingering connection references). It could probably be worked around though

Our hack does not work for the general case: we are using a single thread and only a single connection. Trac itself has to be thread-safe, so it needs to use a new connection per thread and then manage all database interaction on that single connection.

Anyway, thanks for the link to the Trac 0.12 implementation.

[Russ Tyndall]

I added a note to the wiki page about the trac 11 versions.

Cheers, Russ