Modify

Opened 13 years ago

Closed 13 years ago

#8363 closed defect (wontfix)

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

Reported by: mal@… Owned by: Russ Tyndall
Priority: normal Component: TimingAndEstimationPlugin
Severity: normal Keywords: waiting-for-feedback
Cc: Trac Release: 0.11

Description

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.

Attachments (0)

Change History (7)

comment:1 Changed 13 years ago by 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?

comment:2 Changed 13 years ago by Russ Tyndall

Keywords: waiting-for-feedback added

[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.

comment:3 in reply to:  1 Changed 13 years ago by 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.

comment:4 in reply to:  2 Changed 13 years ago by 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 !

comment:5 Changed 13 years ago by Russ Tyndall

Your welcome !

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

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

comment:6 in reply to:  5 Changed 13 years ago by anonymous

Replying to bobbysmith007:

Your welcome !

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

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.

comment:7 Changed 13 years ago by Russ Tyndall

Resolution: wontfix
Status: newclosed

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

Cheers, Russ

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Russ Tyndall.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.