From 4b37ded2897c3ae9384ecdd6209699a0fdc513a3 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 12 May 2023 23:42:09 -0400 Subject: 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 --- doc/build/changelog/unreleased_20/9777.rst | 11 +++++ lib/sqlalchemy/orm/strategies.py | 73 ++++++++++++------------------ test/orm/test_options.py | 19 ++++---- 3 files changed, 48 insertions(+), 55 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/9777.rst diff --git a/doc/build/changelog/unreleased_20/9777.rst b/doc/build/changelog/unreleased_20/9777.rst new file mode 100644 index 000000000..0f043f9be --- /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 8e06c4f59..96b1d053e 100644 --- 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 47ffedb07..3b088b998 100644 --- 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): -- cgit v1.2.1