From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Wed, 20 Feb 2019 00:46:17 +0000 (-0500)
Subject: Ensure _simple_lazy_clause bind names are fixed before cloning
X-Git-Tag: rel_1_2_19~13
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4d2bd4a78fc372c4e08c463cd58dffd1aa891118;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Ensure _simple_lazy_clause bind names are fixed before cloning

Fixed a regression in 1.2 due to the introduction of baked queries for
relationship lazy loaders, where a race condition is created during the
generation of the "lazy clause" which occurs within a memoized attribute. If
two threads initialize the memoized attribute concurrently, the baked query
could be generated with bind parameter keys that are then replaced with new
keys by the next run, leading to a lazy load query that specifies the
related criteria as ``None``. The fix establishes that the parameter names
are fixed before the new clause and parameter objects are generated, so that
the names are the same every time.

Fixes: #4507
Change-Id: I605b824e028c87bc20ca8c2577227cdf6a591064
(cherry picked from commit d71f34cb2a28e8dfc410bc5bec68372be6d7c333)
---

diff --git a/doc/build/changelog/unreleased_12/4507.rst b/doc/build/changelog/unreleased_12/4507.rst
new file mode 100644
index 0000000000..39b37b93d4
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4507.rst
@@ -0,0 +1,13 @@
+.. change::
+   :tags: bug, orm
+   :tickets: 4507
+
+   Fixed a regression in 1.2 due to the introduction of baked queries for
+   relationship lazy loaders, where a race condition is created during the
+   generation of the "lazy clause" which occurs within a memoized attribute. If
+   two threads initialize the memoized attribute concurrently, the baked query
+   could be generated with bind parameter keys that are then replaced with new
+   keys by the next run, leading to a lazy load query that specifies the
+   related criteria as ``None``. The fix establishes that the parameter names
+   are fixed before the new clause and parameter objects are generated, so that
+   the names are the same every time.
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 0d805f903b..38bfbcc596 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -587,6 +587,9 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots):
 
         def visit_bindparam(bindparam):
             bindparam.unique = False
+        visitors.traverse(criterion, {}, {"bindparam": visit_bindparam})
+
+        def visit_bindparam(bindparam):
             if bindparam._identifying_key in bind_to_col:
                 params.append(
                     (
diff --git a/test/ext/test_baked.py b/test/ext/test_baked.py
index d46bd6cf23..2ae33de896 100644
--- a/test/ext/test_baked.py
+++ b/test/ext/test_baked.py
@@ -1266,6 +1266,21 @@ class LazyLoaderTest(testing.AssertsCompiledSQL, BakedTest):
             ),
         )
 
+    def test_simple_lazy_clause_no_race_on_generate(self):
+        User, Address = self._o2m_fixture()
+
+        expr1, paramdict1 = (
+            User.addresses.property._lazy_strategy._simple_lazy_clause
+        )
+
+        # delete the attr, as though a concurrent thread is also generating it
+        del User.addresses.property._lazy_strategy._simple_lazy_clause
+        expr2, paramdict2 = (
+            User.addresses.property._lazy_strategy._simple_lazy_clause
+        )
+
+        eq_(paramdict1, paramdict2)
+
     # additional tests:
     # 1. m2m w lazyload
     # 2. o2m lazyload where m2o backrefs have an eager load, test