Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

#10938 closed defect (fixed)

[PATCH] Incorrect database connection with multiple envs and nested queries

Reported by: juha.mustonen@… Owned by: Christian Boos
Priority: normal Milestone: 0.12.5
Component: database backend Version: 0.12.3
Severity: major Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

When component needs to load components from another environment, thread local variable db can point to incorrect database causing various problems from incorrect data to deadlocks.

The patch included with the ticket solves the issue by setting data into env specific fields. Patch can be applied on Trac 0.12.3.

Test script:

from trac.env import open_environment

env = open_environment(env_path='/var/www/trac/projects/projectx/', use_cache=True)

@env.with_transaction()
def func1(db1):
    cursor1 = db1.cursor()
    cursor1.execute('SELECT DATABASE()')
    env.log.info("cur1 db: %s" % cursor1.fetchone())

    env2 = open_environment(env_path='/var/www/trac/projects/projecty/', use_cache=True)

    @env2.with_transaction()
    def func2(db2):
        cursor2 = db2.cursor()
        cursor2.execute('SELECT DATABASE()')
        env.log.info("cur2 db: %s" % cursor2.fetchone())

        cursor1.execute('SELECT DATABASE()')
        env.log.info("cur1 db: %s" % cursor1.fetchone())

Output:

INFO: cur1 db: projectx
INFO: cur2 db: projectx
INFO: cur1 db: projectx

Expected output:

INFO: cur1 db: projectx
INFO: cur2 db: projecty
INFO: cur1 db: projectx

Attachments (3)

thread_env_local.patch (3.3 KB ) - added by juha.mustonen@… 12 years ago.
Patch for having env based ThreadLocal
t10938-per-env-_transaction_local.diff (6.3 KB ) - added by Christian Boos 12 years ago.
have one _transaction_local per environment, instead of a global one
10938-dbmgr-transaction-local-r11463.patch (5.6 KB ) - added by Remy Blank 12 years ago.
Move _transaction_local into DatabaseManager.

Download all attachments as: .zip

Change History (15)

by juha.mustonen@…, 12 years ago

Attachment: thread_env_local.patch added

Patch for having env based ThreadLocal

comment:1 by Christian Boos, 12 years ago

Milestone: 0.12.5
Owner: set to Christian Boos
Status: newassigned

You're right, as far as our db "transactions" are concerned, we assumed that for a given request (which might correspond to a thread) we're only dealing with one environment, hence one database. But this might not always be true in some multi-environment/multi-project setups (or per chance are you still using some relic of my old InterTrac branch? ;-) ).

Now as for the fix, can't we do something much simpler, like having a ThreadLocal instance attached to the Environment?

comment:2 by juha.mustonen@…, 12 years ago

Indeed. We're having a multienvironment setup where using the components from another env is somewhat common/requirement, and therefore stumbled with the issue every now and then.

As for the patch, I'm sure you can come up with something more nifty solution - I was aiming for small patch.

One thing that was missing from the patch is the CacheManager that should be environment-aware as well:

class CacheManager(Component):
    """Cache manager."""

    required = True
    
    def __init__(self):
        self._cache = {}
-       self._local = ThreadLocal(meta=None, cache=None)
+       self._local = ThreadEnvLocal(self.env.path.split('/')[-1], meta=None, cache=None)
        self._lock = threading.RLock()

Also, it may be better API-wise to pass whole environment object instead of environment name (assuming this approach is used).

comment:3 by Remy Blank, 12 years ago

Interesting case indeed, which we hadn't anticipated. Attaching the thread-local to the environment sounds like a nice and simple solution (and even so obvious in retrospect that we should have implemented it in the first place :).

in reply to:  description comment:4 by Christian Boos, 12 years ago

Replying to juha.mustonen@…:

When component needs to load components from another environment, thread local variable db can point to incorrect database causing various problems from incorrect data to deadlocks.

Speaking of deadlocks, I hope you realize that you will get them occasionally if you plan to access a different db from within a transaction, even with the fix:

  • thread A
    • transaction on env1 (lock)
      • nested transaction on env2 (waiting for lock)
  • thread B
    • transaction on env2 (lock)
      • nested transaction on env1 (waiting for … oops)

That might not happens for read queries, as we try to "exhaust" the cursors (implicit fetchall) so we should never hold a read lock while doing something else. But even this might go wrong, I suppose. This morning I realized that doing a hotcopy was a good way to make the database is locked errors reproducible, with any request happening during that period (at the occasion of session.save()). So read locks can be held for a long time…

by Christian Boos, 12 years ago

have one _transaction_local per environment, instead of a global one

comment:5 by Christian Boos, 12 years ago

I tried to attach the _transaction_local to the DatabaseManager, as it looked better from an encapsulation point of view, however trying to fix the tests was getting horrendous, so I went back to my initial idea: see t10938-per-env-_transaction_local.diff.

by Remy Blank, 12 years ago

Move _transaction_local into DatabaseManager.

comment:6 by Remy Blank, 12 years ago

Now that you mention it, DatabaseManager would indeed be a better location. How about 10938-dbmgr-transaction-local-r11463.patch? All tests pass here.

comment:7 by Christian Boos, 12 years ago

Ah MockDatabaseManager, that was the trick! I tried to get a real one… Yes, your patch is the winner.

Still comment:2 to go…

in reply to:  2 ; comment:8 by Christian Boos, 12 years ago

Replying to juha.mustonen@…:

[…] One thing that was missing from the patch is the CacheManager that should be environment-aware as well:

class CacheManager(Component):
    """Cache manager."""

    required = True
    
    def __init__(self):
        self._cache = {}
-       self._local = ThreadLocal(meta=None, cache=None)
+       self._local = ThreadEnvLocal(self.env.path.split('/')[-1], meta=None, cache=None)
        self._lock = threading.RLock()

But wait, I don't see the point here: a CacheManager is environment specific (it's a Component). So each self._local in the above is different from the other, no?

in reply to:  8 comment:9 by Remy Blank, 12 years ago

Replying to cboos:

But wait, I don't see the point here: a CacheManager is environment specific (it's a Component). So each self._local in the above is different from the other, no?

I was going to write exactly that, too. Juha, can you please describe the cache-related symptoms that you see?

comment:10 by juha.mustonen@…, 12 years ago

To be honest, we haven't encountered any cache related issues so far. ThreadLocal keyword caught my eye, that's why :)

comment:11 by Remy Blank, 12 years ago

Ok, we seem to have a winner, then. Patch applied on 0.12-stable in [11467]. Now on to merging this forward…

comment:12 by Remy Blank, 12 years ago

Resolution: fixed
Status: assignedclosed

Merged forward to 1.0-stable in [11468] and trunk in [11469].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.