remove "aliased class pool" caching approach

Modified the ``JoinedLoader`` implementation to use a simpler approach in
one particular area where it previously used a cached structure that would
be shared among threads. The rationale is to avoid a potential race
condition which is suspected of being the cause of a particular crash
that's been reported multiple times. The cached structure in question is
still ultimately "cached" via the compiled SQL cache, so a performance
degradation is not anticipated.

The change also modifies the tests for None in context.secondary
to ensure no None values are in this list, as this is suspected
as being the immediate cause of the issue in #9777.  The cached
AliasedClass thing is suspected as being the origination of the
cause, as under high concurrency many threads might all access
that AliasedClass immediately, which seems a reasonable place
that the "adapter returning None" symptom might be originating.

As of yet we don't have a self-contained reproducer for the
issue, some initial attempts with threads are not showing any
issue.

Fixes: #9777
Change-Id: I967588f280796c413e629b55b8d97d40c1164248

diff --git a/doc/build/changelog/unreleased_20/9777.rst b/doc/build/changelog/unreleased_20/9777.rst
new file mode 100644 (file)
index 0000000..0f043f9
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/9777.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 9777
+
+    Modified the ``JoinedLoader`` implementation to use a simpler approach in
+    one particular area where it previously used a cached structure that would
+    be shared among threads. The rationale is to avoid a potential race
+    condition which is suspected of being the cause of a particular crash
+    that's been reported multiple times. The cached structure in question is
+    still ultimately "cached" via the compiled SQL cache, so a performance
+    degradation is not anticipated.

diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 8e06c4f5986bcc3c1af90c6b1d35cb3d7fff173e..96b1d053e09178ca4f512144c17caf14c1803b25 100644 (file)
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -2097,12 +2097,11 @@ class JoinedLoader(AbstractRelationshipLoader):
 
     """
 
-    __slots__ = "join_depth", "_aliased_class_pool"
+    __slots__ = "join_depth"
 
     def __init__(self, parent, strategy_key):
         super().__init__(parent, strategy_key)
         self.join_depth = self.parent_property.join_depth
-        self._aliased_class_pool = []
 
     def init_class_attribute(self, mapper):
         self.parent_property._get_strategy(
@@ -2215,14 +2214,19 @@ class JoinedLoader(AbstractRelationshipLoader):
             chained_from_outerjoin=chained_from_outerjoin,
         )
 
-        if with_poly_entity is not None and None in set(
-            compile_state.secondary_columns
-        ):
-            raise sa_exc.InvalidRequestError(
-                "Detected unaliased columns when generating joined "
-                "load.  Make sure to use aliased=True or flat=True "
-                "when using joined loading with with_polymorphic()."
-            )
+        has_nones = util.NONE_SET.intersection(compile_state.secondary_columns)
+
+        if has_nones:
+            if with_poly_entity is not None:
+                raise sa_exc.InvalidRequestError(
+                    "Detected unaliased columns when generating joined "
+                    "load.  Make sure to use aliased=True or flat=True "
+                    "when using joined loading with with_polymorphic()."
+                )
+            else:
+                compile_state.secondary_columns = [
+                    c for c in compile_state.secondary_columns if c is not None
+                ]
 
     def _init_user_defined_eager_proc(
         self, loadopt, compile_state, target_attributes
@@ -2308,40 +2312,6 @@ class JoinedLoader(AbstractRelationshipLoader):
         add_to_collection = context.primary_columns
         return user_defined_adapter, adapter, add_to_collection
 
-    def _gen_pooled_aliased_class(self, context):
-        # keep a local pool of AliasedClass objects that get re-used.
-        # we need one unique AliasedClass per query per appearance of our
-        # entity in the query.
-
-        if inspect(self.entity).is_aliased_class:
-            alt_selectable = inspect(self.entity).selectable
-        else:
-            alt_selectable = None
-
-        key = ("joinedloader_ac", self)
-        if key not in context.attributes:
-            context.attributes[key] = idx = 0
-        else:
-            context.attributes[key] = idx = context.attributes[key] + 1
-
-        if idx >= len(self._aliased_class_pool):
-            to_adapt = orm_util.AliasedClass(
-                self.mapper,
-                alias=alt_selectable._anonymous_fromclause(flat=True)
-                if alt_selectable is not None
-                else None,
-                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)
-
-        return self._aliased_class_pool[idx]
-
     def _generate_row_adapter(
         self,
         compile_state,
@@ -2359,7 +2329,20 @@ class JoinedLoader(AbstractRelationshipLoader):
         if with_poly_entity:
             to_adapt = with_poly_entity
         else:
-            to_adapt = self._gen_pooled_aliased_class(compile_state)
+            insp = inspect(self.entity)
+            if insp.is_aliased_class:
+                alt_selectable = insp.selectable
+            else:
+                alt_selectable = None
+
+            to_adapt = orm_util.AliasedClass(
+                self.mapper,
+                alias=alt_selectable._anonymous_fromclause(flat=True)
+                if alt_selectable is not None
+                else None,
+                flat=True,
+                use_mapper_path=True,
+            )
 
         to_adapt_insp = inspect(to_adapt)
 

diff --git a/test/orm/test_options.py b/test/orm/test_options.py
index 47ffedb07c6325d6cc0e639942716df13c6564c3..3b088b99867a7bcaa080eb69d76e4b0fb841bd4b 100644 (file)
--- a/test/orm/test_options.py
+++ b/test/orm/test_options.py
@@ -28,6 +28,7 @@ from sqlalchemy.orm import undefer
 from sqlalchemy.orm import util as orm_util
 from sqlalchemy.orm import with_polymorphic
 from sqlalchemy.testing import fixtures
+from sqlalchemy.testing import is_not
 from sqlalchemy.testing.assertions import assert_raises_message
 from sqlalchemy.testing.assertions import AssertsCompiledSQL
 from sqlalchemy.testing.assertions import emits_warning
@@ -1288,20 +1289,18 @@ class PickleTest(fixtures.MappedTest):
     def test_pickle_relationship_loader(self, user_address_fixture):
         User, Address = user_address_fixture
 
-        for i in range(3):
-            opt = joinedload(User.addresses)
-
-            q1 = fixture_session().query(User).options(opt)
-            c1 = q1._compile_context()
+        opt = joinedload(User.addresses)
 
-            pickled = pickle.dumps(opt)
+        pickled = pickle.dumps(opt)
 
-            opt2 = pickle.loads(pickled)
+        opt2 = pickle.loads(pickled)
 
-            q2 = fixture_session().query(User).options(opt2)
-            c2 = q2._compile_context()
+        is_not(opt, opt2)
+        assert isinstance(opt, Load)
+        assert isinstance(opt2, Load)
 
-            eq_(c1.attributes, c2.attributes)
+        for k in opt.__slots__:
+            eq_(getattr(opt, k), getattr(opt2, k))
 
 
 class LocalOptsTest(PathTest, QueryTest):