Skip to content

Commit aff320d

Browse files
committed
PYCBC-1821: Guard lazy Transactions init against concurrent first access
Changes -------- * Guard the check-then-set in ClusterImpl.transactions and AsyncClusterImpl.transactions with a double-checked threading.Lock, so concurrent first access can no longer construct and orphan multiple transactions instances for one cluster; an orphan still owns cleanup threads and a client-record registration, and closing it on GC can deadlock the single core IO thread against the GIL * Tear down self._transactions in both close_connection() implementations, swapping it out under that same lock so the instance is closed deterministically rather than from a GC finalizer; an unlocked read could observe None while a concurrent first access was still inside Transactions() and skip teardown altogether, stranding a live instance on a closed cluster with GC as its only close path, and the swap also makes a concurrent double-close idempotent * Keep TransactionsLogic.close() itself outside the lock, since it is a network round-trip and holding the lock across it would stall a concurrent first access behind teardown; with PYCBC-1798 the GIL is released across it, so it blocks only the calling thread * Wrap that close() in try/except so a failure cannot skip the connection close that follows and leak the core connection and its IO thread; logged rather than swallowed, since a failed close leaves cleanup threads and a client-record registration live * Accept skip_connect in AsyncClientAdapter and forward it from AsyncClusterImpl, matching the existing sync ClientAdapter path, so a cluster can be built for tests without a C++ core connection * Add regression tests stampeding NUM_THREADS threads at an unconnected Cluster/AsyncCluster's transactions property, asserting every thread observes the same instance, exactly one was constructed, and no thread hangs * Add regression tests racing cluster.close() against a first access on both bindings, asserting the instance published by the racing access is closed and self._transactions is cleared; the async case stubs AsyncClientAdapter.close_connection, which otherwise chains a connect request ahead of the close under skip_connect Change-Id: I70bc7d85ca1feda2d482a2e43a099eec42f951c8 Reviewed-on: https://review.couchbase.org/c/couchbase-python-client/+/251051 Reviewed-by: Dimitris Christodoulou <dimitris.christodoulou@couchbase.com> Reviewed-by: Sergey Avseyev <sergey.avseyev@gmail.com> Tested-by: Build Bot <build@couchbase.com> (cherry picked from commit d0a0529) Reviewed-on: https://review.couchbase.org/c/couchbase-python-client/+/251711
1 parent b0ce0a8 commit aff320d

6 files changed

Lines changed: 376 additions & 8 deletions

File tree

acouchbase/logic/client_adapter.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ class AsyncClientAdapter:
5151
def __init__(self,
5252
connect_req: CreateConnectionRequest,
5353
loop: Optional[AbstractEventLoop] = None,
54-
loop_validator: Optional[Callable[[Optional[AbstractEventLoop]], AbstractEventLoop]] = None
54+
loop_validator: Optional[Callable[[Optional[AbstractEventLoop]], AbstractEventLoop]] = None,
55+
**kwargs: Any
5556
) -> None:
5657
num_io_threads = connect_req.options.get('num_io_threads', None)
5758
self._connection = pycbc_connection(num_io_threads) if num_io_threads is not None else pycbc_connection()
@@ -64,7 +65,9 @@ def __init__(self,
6465
self._close_ft: Optional[Future[None]] = None
6566
self._connect_ft: Optional[Future[None]] = None
6667
self._closed = False
67-
self._create_connection()
68+
# for testing we sometimes want to skip the actual C++ core connection
69+
if not (kwargs.get('skip_connect', None) == 'TEST_SKIP_CONNECT'):
70+
self._create_connection()
6871

6972
@property
7073
def binding_map(self) -> BindingMap:

acouchbase/logic/cluster_impl.py

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515

1616
from __future__ import annotations
1717

18+
import logging
19+
import threading
1820
import time
1921
from asyncio import AbstractEventLoop, sleep
2022
from typing import (TYPE_CHECKING,
@@ -52,9 +54,12 @@
5254
UpdateCredentialsRequest,
5355
WaitUntilReadyRequest)
5456

57+
log = logging.getLogger(__name__)
58+
5559

5660
class AsyncClusterImpl:
5761
def __init__(self, connstr: str, *options: object, **kwargs: object) -> None:
62+
skip_connect = kwargs.pop('skip_connect', None)
5863
loop: Optional[AbstractEventLoop] = kwargs.pop('loop', None)
5964
loop_validator = kwargs.pop('loop_validator', None)
6065
kwargs['_default_timeouts'] = pycbc_connection.pycbc_get_default_timeouts()
@@ -65,12 +70,15 @@ def __init__(self, connstr: str, *options: object, **kwargs: object) -> None:
6570
# A connection is made when we create the client adapter, but it is an async operation that we cannot await
6671
# b/c the call needs to happen when we initialize a cluster (new cluster -> new client adapter). We await
6772
# the create connection future in whichever operation comes next.
68-
self._client_adapter = AsyncClientAdapter(connect_request, loop=loop, loop_validator=loop_validator)
73+
self._client_adapter = AsyncClientAdapter(
74+
connect_request, loop=loop, loop_validator=loop_validator, skip_connect=skip_connect)
6975
self._cluster_settings.set_observability_cluster_labels_callable(
7076
self._client_adapter.binding_map.op_map[ClusterOperationType.GetClusterLabels.value])
7177
self._request_builder = ClusterRequestBuilder()
7278
self._cluster_info: Optional[ClusterInfoResult] = None
7379
self._transactions: Optional[Transactions] = None
80+
# Guards the lazy init of self._transactions in the transactions property
81+
self._transactions_lock = threading.Lock()
7482

7583
@property
7684
def client_adapter(self) -> AsyncClientAdapter:
@@ -149,8 +157,13 @@ def observability_instruments(self) -> ObservabilityInstruments:
149157
@property
150158
def transactions(self) -> Transactions:
151159
"""**INTERNAL**"""
152-
if not self._transactions:
153-
self._transactions = Transactions(self)
160+
# Transactions.__init__ goes straight into the synchronous create_transactions (no executor
161+
# anywhere in the async path), which releases the GIL for a full cluster round-trip.
162+
# Use threading.Lock, not asyncio.Lock, for the same reason.
163+
if self._transactions is None:
164+
with self._transactions_lock:
165+
if self._transactions is None:
166+
self._transactions = Transactions(self)
154167
return self._transactions
155168

156169
def analytics_query(self, req: AnalyticsQueryRequest) -> AnalyticsResult:
@@ -173,6 +186,25 @@ def analytics_query(self, req: AnalyticsQueryRequest) -> AnalyticsResult:
173186

174187
async def close_connection(self) -> None:
175188
"""**INTERNAL**"""
189+
190+
# Swap under the lock, close outside it. An off-loop first access holds the lock
191+
# across Transactions(), so an unlocked read here can see None mid-construction and
192+
# skip teardown, stranding a live instance on a closed cluster with only GC to close
193+
# it. close() is a network round-trip and must not be held under the lock.
194+
with self._transactions_lock:
195+
txns, self._transactions = self._transactions, None
196+
197+
# Ahead of the adapter close: the core cannot remove its client record once the
198+
# connection is gone, and burns the full retry budget failing to.
199+
if txns is not None:
200+
try:
201+
# Synchronous with no async override, so this blocks the loop thread (as in
202+
# < 4.6.0). PYCBC-1798 releases the GIL, so only this thread blocks.
203+
txns.close()
204+
except Exception:
205+
# Must not skip the connection close below and leak the core IO thread.
206+
log.warning('Error closing transactions while closing the cluster.', exc_info=True)
207+
176208
try:
177209
from couchbase.logic.observability import ThresholdLoggingTracer
178210
tracer = self._cluster_settings.tracer.tracer
Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
# Copyright 2016-2026. Couchbase, Inc.
2+
# All Rights Reserved.
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License")
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
16+
"""
17+
Regression test for PYCBC-1821 (the >= 4.6.0 counterpart of PYCBC-1815).
18+
AsyncClusterImpl.transactions was a bare check-then-set, so threads racing the
19+
first access could each construct and orphan their own Transactions instance.
20+
An orphaned instance still owns cleanup threads and a client-record
21+
registration, and closing it on GC can deadlock the single core IO thread
22+
against the GIL (see transactions.cxx's dealloc_transactions).
23+
24+
The property is synchronous and Transactions.__init__ drops the GIL for a full
25+
cluster round-trip, so the racers here are OS threads, not tasks -- the guard
26+
is a threading.Lock for the same reason.
27+
28+
No live cluster is needed. The property never touches the network before
29+
constructing a Transactions object, so the cluster is built with
30+
skip_connect='TEST_SKIP_CONNECT' (same mechanism couchbase/tests/connection_t.py
31+
uses) and exercised purely at the Python layer, with Transactions swapped for a
32+
fake that holds the construction window open long enough to hit the race
33+
reliably.
34+
"""
35+
36+
import threading
37+
38+
import pytest
39+
40+
from acouchbase.cluster import AsyncCluster
41+
from acouchbase.logic.client_adapter import AsyncClientAdapter
42+
from couchbase.auth import PasswordAuthenticator
43+
from couchbase.options import ClusterOptions
44+
45+
NUM_THREADS = 16
46+
# How long the fake Transactions() takes to "construct", wide enough that
47+
# every thread's check-then-set race window stays open for the whole
48+
# stampede on the old unguarded property.
49+
CONSTRUCTION_DELAY = 0.05
50+
51+
52+
class TransactionsPropertyRaceTestSuite:
53+
TEST_MANIFEST = [
54+
'test_close_during_first_access_closes_instance',
55+
'test_concurrent_first_access_returns_single_instance',
56+
]
57+
58+
@pytest.fixture(name='tracking_transactions')
59+
def tracking_transactions_cls(self, monkeypatch):
60+
"""A stand-in for acouchbase.transactions.Transactions.
61+
62+
Counts constructions (lock-guarded), signals once a construction is in
63+
flight, and sleeps briefly so the race window stays open regardless of
64+
thread scheduling.
65+
"""
66+
lock = threading.Lock()
67+
68+
class _Tracker:
69+
instances_created = 0
70+
construction_started = threading.Event()
71+
72+
def __init__(self, cluster):
73+
self.close_count = 0
74+
with lock:
75+
type(self).instances_created += 1
76+
type(self).construction_started.set()
77+
threading.Event().wait(CONSTRUCTION_DELAY)
78+
79+
def close(self):
80+
with lock:
81+
self.close_count += 1
82+
83+
monkeypatch.setattr('acouchbase.logic.cluster_impl.Transactions', _Tracker)
84+
return _Tracker
85+
86+
@pytest.fixture(name='unconnected_cluster')
87+
def cluster_without_core_connection(self, couchbase_config):
88+
conn_string = couchbase_config.get_connection_string()
89+
username, pw = couchbase_config.get_username_and_pw()
90+
# No event loop is touched: skipping the create-connection request means the adapter's
91+
# lazy loop lookup never runs, and the faked Transactions never reads cluster.loop.
92+
return AsyncCluster(conn_string,
93+
ClusterOptions(PasswordAuthenticator(username, pw)),
94+
skip_connect='TEST_SKIP_CONNECT')
95+
96+
@pytest.fixture(name='stub_adapter_close')
97+
def stub_async_adapter_close(self, monkeypatch):
98+
"""Stub out the core half of close_connection().
99+
100+
When not connected, AsyncClientAdapter chains a connect request ahead of the
101+
close (client_adapter.py:301), so under skip_connect the close would establish
102+
the very connection this test skipped. The sync adapter early-outs instead and
103+
needs no stub, so this keeps both tests network-free and asserting only the
104+
lock ordering.
105+
"""
106+
107+
async def _noop(self):
108+
self._closed = True
109+
110+
monkeypatch.setattr(AsyncClientAdapter, 'close_connection', _noop)
111+
112+
@pytest.mark.asyncio
113+
@pytest.mark.usefixtures('stub_adapter_close')
114+
async def test_close_during_first_access_closes_instance(self, tracking_transactions, unconnected_cluster):
115+
result = {}
116+
117+
def touch():
118+
# Off-loop: how the property stays reachable while close() runs on the loop.
119+
result['txns'] = unconnected_cluster.transactions
120+
121+
toucher = threading.Thread(target=touch)
122+
toucher.start()
123+
# Proceed only once the property is inside Transactions(), so close() races it.
124+
assert tracking_transactions.construction_started.wait(timeout=10), 'construction never started'
125+
await unconnected_cluster.close()
126+
toucher.join(timeout=10)
127+
assert not toucher.is_alive(), 'thread failed to join, possible deadlock'
128+
129+
txns = result.get('txns')
130+
assert txns is not None
131+
assert tracking_transactions.instances_created == 1
132+
assert txns.close_count == 1, 'the instance published by the racing first access was never closed'
133+
assert unconnected_cluster._impl._transactions is None, 'a live Transactions outlived cluster.close()'
134+
135+
def test_concurrent_first_access_returns_single_instance(self, tracking_transactions, unconnected_cluster):
136+
barrier = threading.Barrier(NUM_THREADS)
137+
results = [None] * NUM_THREADS
138+
139+
def touch(idx):
140+
barrier.wait()
141+
results[idx] = unconnected_cluster.transactions
142+
143+
threads = [threading.Thread(target=touch, args=(i,)) for i in range(NUM_THREADS)]
144+
for t in threads:
145+
t.start()
146+
for t in threads:
147+
t.join(timeout=10)
148+
assert not t.is_alive(), 'thread failed to join, possible deadlock'
149+
150+
assert all(r is not None for r in results)
151+
first = results[0]
152+
assert all(r is first for r in results), 'racing threads observed different Transactions instances'
153+
assert tracking_transactions.instances_created == 1
154+
155+
156+
class TransactionsPropertyRaceTests(TransactionsPropertyRaceTestSuite):
157+
158+
@pytest.fixture(scope='class', autouse=True)
159+
def manifest_validated(self):
160+
def valid_test_method(meth):
161+
attr = getattr(TransactionsPropertyRaceTests, meth)
162+
return callable(attr) and not meth.startswith('__') and meth.startswith('test')
163+
method_list = [meth for meth in dir(TransactionsPropertyRaceTests) if valid_test_method(meth)]
164+
test_list = set(TransactionsPropertyRaceTestSuite.TEST_MANIFEST).symmetric_difference(method_list)
165+
if test_list:
166+
pytest.fail(f'Test manifest not validated. Missing/extra tests: {test_list}.')

conftest.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,10 @@
108108

109109
_MISC_TESTS = [
110110
"acouchbase/tests/rate_limit_t.py::RateLimitTests",
111-
"couchbase/tests/connection_t.py::ClassicConnectionTests"
111+
"acouchbase/tests/transactions_property_race_t.py::TransactionsPropertyRaceTests",
112+
"couchbase/tests/connection_t.py::ClassicConnectionTests",
112113
"couchbase/tests/rate_limit_t.py::ClassicRateLimitTests",
114+
"couchbase/tests/transactions_property_race_t.py::ClassicTransactionsPropertyRaceTests",
113115
]
114116

115117
_TRACING_TESTS = [

couchbase/logic/cluster_impl.py

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515

1616
from __future__ import annotations
1717

18+
import logging
19+
import threading
1820
import time
1921
from typing import (TYPE_CHECKING,
2022
Any,
@@ -53,6 +55,8 @@
5355
WaitUntilReadyRequest)
5456
from couchbase.serializer import Serializer
5557

58+
log = logging.getLogger(__name__)
59+
5660

5761
class ClusterImpl:
5862
def __init__(self, connstr: str, *options: object, **kwargs: object) -> None:
@@ -68,6 +72,8 @@ def __init__(self, connstr: str, *options: object, **kwargs: object) -> None:
6872
self._request_builder = ClusterRequestBuilder()
6973
self._cluster_info: Optional[ClusterInfoResult] = None
7074
self._transactions: Optional[Transactions] = None
75+
# Guards the lazy init of self._transactions in the transactions property
76+
self._transactions_lock = threading.Lock()
7177

7278
@property
7379
def client_adapter(self) -> ClientAdapter:
@@ -143,8 +149,10 @@ def observability_instruments(self) -> ObservabilityInstruments:
143149
@property
144150
def transactions(self) -> Transactions:
145151
"""**INTERNAL**"""
146-
if not self._transactions:
147-
self._transactions = Transactions(self)
152+
if self._transactions is None:
153+
with self._transactions_lock:
154+
if self._transactions is None:
155+
self._transactions = Transactions(self)
148156
return self._transactions
149157

150158
def analytics_query(self, req: AnalyticsQueryRequest) -> AnalyticsResult:
@@ -165,6 +173,24 @@ def analytics_query(self, req: AnalyticsQueryRequest) -> AnalyticsResult:
165173

166174
def close_connection(self) -> None:
167175
"""**INTERNAL**"""
176+
177+
# Swap under the lock, close outside it. A first access holds the lock across
178+
# Transactions(), so an unlocked read here can see None mid-construction and skip
179+
# teardown, stranding a live instance on a closed cluster with only GC to close it.
180+
# close() is a network round-trip and must not be held under the lock.
181+
with self._transactions_lock:
182+
txns, self._transactions = self._transactions, None
183+
184+
# Ahead of the adapter close: the core cannot remove its client record once the
185+
# connection is gone, and burns the full retry budget failing to.
186+
if txns is not None:
187+
try:
188+
# Synchronous, but PYCBC-1798 releases the GIL, so only this thread blocks.
189+
txns.close()
190+
except Exception:
191+
# Must not skip the connection close below and leak the core IO thread.
192+
log.warning('Error closing transactions while closing the cluster.', exc_info=True)
193+
168194
try:
169195
from couchbase.logic.observability import ThresholdLoggingTracer
170196
tracer = self._cluster_settings.observability_instruments.tracer.tracer

0 commit comments

Comments
 (0)