Skip to content

Commit c513b39

Browse files
lyushherparthea
andauthored
fix(ndb): avoid unbound lock in delete callback (#17969)
## Summary This PR fixes a `NameError` that occurs when `ndb.delete()` is called with `use_datastore=False` inside a transaction while the global cache is enabled. The transaction completion callback can reference `lock` even though no lock is acquired when datastore writes are disabled. ## Changes - Avoid registering the unlock callback when no lock has been acquired. - Add a regression test covering this scenario. ## Testing ```bash python -m pytest tests/unit/test__datastore_api.py -v ``` All 78 tests passed. Fixes #17926 --------- Co-authored-by: Anthonios Partheniou <partheniou@google.com>
1 parent ad99ed1 commit c513b39

2 files changed

Lines changed: 54 additions & 7 deletions

File tree

packages/google-cloud-ndb/google/cloud/ndb/_datastore_api.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,7 @@ def delete(key, options):
434434
use_global_cache = context._use_global_cache(key, options)
435435
use_datastore = context._use_datastore(key, options)
436436
transaction = context.transaction
437+
lock = None
437438

438439
if use_global_cache:
439440
cache_key = _cache.global_cache_key(key)
@@ -450,17 +451,18 @@ def delete(key, options):
450451
yield batch.delete(key)
451452

452453
if use_global_cache:
453-
if transaction:
454+
if lock is not None:
455+
if transaction:
454456

455-
def callback():
456-
_cache.global_unlock_for_write(cache_key, lock).result()
457+
def callback():
458+
_cache.global_unlock_for_write(cache_key, lock).result()
457459

458-
context.call_on_transaction_complete(callback)
460+
context.call_on_transaction_complete(callback)
459461

460-
elif use_datastore:
461-
yield _cache.global_unlock_for_write(cache_key, lock)
462+
else:
463+
yield _cache.global_unlock_for_write(cache_key, lock)
462464

463-
else:
465+
elif not use_datastore:
464466
yield _cache.global_delete(cache_key)
465467

466468

packages/google-cloud-ndb/tests/unit/test__datastore_api.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,26 @@ def test_cache_enabled(Batch, global_cache):
882882

883883
assert not global_cache.get([cache_key])[0]
884884

885+
@staticmethod
886+
@mock.patch("google.cloud.ndb._datastore_api._NonTransactionalCommitBatch")
887+
def test_lock_not_acquired(Batch, global_cache):
888+
key = key_module.Key("SomeKind", 1)
889+
cache_key = _cache.global_cache_key(key._key)
890+
global_cache.set({cache_key: b"foo"})
891+
892+
batch = Batch.return_value
893+
batch.delete.return_value = future_result(None)
894+
895+
with mock.patch(
896+
"google.cloud.ndb._datastore_api._cache.global_lock_for_write",
897+
return_value=future_result(None),
898+
):
899+
future = _api.delete(key._key, _options.Options())
900+
901+
assert future.result() is None
902+
batch.delete.assert_called_once_with(key._key)
903+
assert global_cache.get([cache_key]) == [b"foo"]
904+
885905
@staticmethod
886906
@mock.patch("google.cloud.ndb._datastore_api._NonTransactionalCommitBatch")
887907
def test_w_transaction(Batch, global_cache):
@@ -921,6 +941,31 @@ def test_without_datastore(Batch, global_cache):
921941

922942
assert global_cache.get([cache_key]) == [None]
923943

944+
@staticmethod
945+
@mock.patch("google.cloud.ndb._datastore_api._NonTransactionalCommitBatch")
946+
def test_without_datastore_with_transaction(Batch, global_cache):
947+
context = context_module.get_context()
948+
callbacks = []
949+
950+
with context.new(
951+
transaction=b"abc123", transaction_complete_callbacks=callbacks
952+
).use():
953+
key = key_module.Key("SomeKind", 1)
954+
cache_key = _cache.global_cache_key(key._key)
955+
global_cache.set({cache_key: b"foo"})
956+
957+
batch = Batch.return_value
958+
batch.delete.side_effect = Exception("Shouldn't use Datastore")
959+
960+
future = _api.delete(
961+
key._key,
962+
_options.Options(use_datastore=False),
963+
)
964+
assert future.result() is None
965+
966+
assert callbacks == []
967+
assert global_cache.get([cache_key]) == [None]
968+
924969
@staticmethod
925970
@mock.patch("google.cloud.ndb._datastore_api._NonTransactionalCommitBatch")
926971
def test_cache_disabled(Batch, global_cache):

0 commit comments

Comments
 (0)