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

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

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

[juha.mustonen@…]

Patch for having env based ThreadLocal

[Christian Boos]

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?

[juha.mustonen@…]

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

[Remy Blank]

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 :).

[Christian Boos]

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…

[Christian Boos]

have one _transaction_local per environment, instead of a global one

[Christian Boos]

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

[Remy Blank]

Move _transaction_local into DatabaseManager.

[Remy Blank]

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

[Christian Boos]

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

Still comment:2 to go…

[Christian Boos]

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?

[Remy Blank]

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?

[juha.mustonen@…]

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

[Remy Blank]

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

[Remy Blank]

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