race condition in joined eager load perf enhancement

[Mike Bayer]

e.g. #3915

test:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()


class A(Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)
    bs = relationship("B")

class B(Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)
    a_id = Column(ForeignKey('a.id'))

e = create_engine("sqlite:///foo.db", echo=False)
Base.metadata.create_all(e)


import threading
import random
import time
import traceback

def work(e):
    s = Session(e)
    while True:
        try:
            s.query(A).options(joinedload(A.bs)).all()
        except Exception as e:
            traceback.print_exc()
            print "Exception: %s" % e
        finally:
            time.sleep(random.random())

def chaos():
    while True:
        time.sleep(.001)
        A.bs.property._get_strategy((("lazy", "joined"),))._aliased_class_pool[:] = []

s = Session(e)
s.add_all([A(bs=[B(), B(), B()])])
s.commit()


threads = [threading.Thread(target=work, args=(e, )) for i in range(20)]
chaos = threading.Thread(target=chaos)

for t in threads:
    t.start()
chaos.start()

traceback

#!

Traceback (most recent call last):
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/query.py", line 2679, in all
  File "test.py", line 31, in work
    s.query(A).options(joinedload(A.bs)).all()
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/query.py", line 2679, in all
yes
    return list(self)
    return list(self)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 90, in instances
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/query.py", line 2831, in __iter__
    return self._execute_and_instances(context)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/query.py", line 2854, in _execute_and_instances
    result = conn.execute(querycontext.statement, self._params)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/base.py", line 945, in execute
    util.raise_from_cause(err)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/util/compat.py", line 203, in raise_from_cause
    reraise(type(exception), exception, tb=exc_tb, cause=cause)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 57, in instances
    for query_entity in query._entities
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/query.py", line 3686, in row_processor
    polymorphic_discriminator=self._polymorphic_discriminator
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 330, in _instance_processor
    context, path, mapper, result, adapter, populators)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/interfaces.py", line 532, in create_row_processor
    return meth(self, multiparams, params)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/sql/elements.py", line 269, in _execute_on_connection
    mapper, result, adapter, populators)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/strategies.py", line 1594, in create_row_processor
    eager_adapter)
    return connection._execute_clauseelement(self, multiparams, params)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 318, in _instance_processor
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/base.py", line 1053, in _execute_clauseelement
    col = adapter.columns[col]
    compiled_sql, distilled_params
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/sql/util.py", line 713, in __getitem__
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/base.py", line 1189, in _execute_context
    context)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/base.py", line 1393, in _handle_dbapi_exception
    exc_info
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/util/compat.py", line 203, in raise_from_cause
    reraise(type(exception), exception, tb=exc_tb, cause=cause)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/base.py", line 1182, in _execute_context
    context)
    self.parent.include_fn and not self.parent.include_fn(key)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/util.py", line 326, in _include_fn
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/default.py", line 504, in do_execute
    entity = elem._annotations.get('parentmapper', None)
AttributeError: 'NoneType' object has no attribute '_annotations'
Exception: 'NoneType' object has no attribute '_annotations'
    cursor.execute(statement, parameters)

[Mike Bayer]

OK so in the example columns from "b" are in some cases failing to be adapted, because ClauseAdapter._corresponding_column() is failing. It fails because selectable.corresponding_column fails, which is because the race is the .c collection on the Alias object inside of AliasedClass.

we can narrow down the case that is racing to the _populate_column_collection() of the Alias. If we do alias.c to pre-load this inside the AliasedClass, that resolves. Or this mutex also resolves:

diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py
index b69d667..19d956d 100644
--- a/lib/sqlalchemy/sql/selectable.py
+++ b/lib/sqlalchemy/sql/selectable.py
@@ -311,7 +311,7 @@ class HasSuffixes(object):
         self._suffixes = self._suffixes + tuple(
             [(_literal_as_text(p, warn=False), dialect) for p in suffixes])

-
+import threading
 class FromClause(Selectable):
     """Represent an element that can be used within the ``FROM``
     clause of a ``SELECT`` statement.
@@ -330,6 +330,7 @@ class FromClause(Selectable):


     """
+    mutex = threading.Lock()
     __visit_name__ = 'fromclause'
     named_with_column = False
     _hide_froms = []
@@ -681,9 +682,13 @@ class FromClause(Selectable):

         """

-        if '_columns' not in self.__dict__:
-            self._init_collections()
-            self._populate_column_collection()
+        self.mutex.acquire()
+        try:
+            if '_columns' not in self.__dict__:
+                self._init_collections()
+                self._populate_column_collection()
+        finally:
+            self.mutex.release()
         return self._columns.as_immutable()

     @_memoized_property

[Mike Bayer]

it's not totally clear if we can generalize an issue to the .c. collection of FromClause overall; I wasn't able to get a test to show unreasonable races at that specific of a level, and the races I could show aren't fixable internally to FromClause.

in this case, the most simple and localized fix is to pre-load the .c. collection within the strategy, so that .c. is fully set up before the object is shared among threads:

diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index caf0da3..c70994e 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -1309,6 +1309,10 @@ class JoinedLoader(AbstractRelationshipLoader):
                 self.mapper,
                 flat=True,
                 use_mapper_path=True)
+            # load up the .columns collection on the Alias() before
+            # the object becomes shared among threads.  this prevents
+            # races for column identities.
+            inspect(to_adapt).selectable.c

             self._aliased_class_pool.append(to_adapt)

probably best to start with this.

[Mike Bayer]

https://gerrit.sqlalchemy.org/#/c/353/

[Mike Bayer]

Pre-load alias.c within JoinedEagerLoader cached AliasedClass

Fixed a race condition which could occur under threaded environments as a result of the caching added via 3915. An internal collection of Column objects could be regenerated on an alias object inappropriately, confusing a joined eager loader when it attempts to render SQL and collect results and resulting in an attribute error. The collection is now generated up front before the alias object is cached and shared among threads.

Change-Id: I97d5b205992d38af8d2b4307178a15c086ef9993 Fixes: #3947

→ <<cset f214f4d4f46d>>

[Mike Bayer]

Pre-load alias.c within JoinedEagerLoader cached AliasedClass

Fixed a race condition which could occur under threaded environments as a result of the caching added via 3915. An internal collection of Column objects could be regenerated on an alias object inappropriately, confusing a joined eager loader when it attempts to render SQL and collect results and resulting in an attribute error. The collection is now generated up front before the alias object is cached and shared among threads.

Change-Id: I97d5b205992d38af8d2b4307178a15c086ef9993 Fixes: #3947 (cherry picked from commit f214f4d4f46de24008c63f2e034329a64f510833)

→ <<cset 7a6cc6e897d8>>