| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Note This is a comment from Claude, an AI tool. @rlamb ran a multi-agent review of this PR and asked Claude to post this finding with a test. Problem: wait_stopped() does not correctly handle cancellation of its callerAsyncRepeatingTask.wait_stopped() uses await task (ldclient/impl/aio/concurrency.py, line 232). This statement makes the polling task the _fut_waiter of the caller. If asyncio cancels the caller of stop(), two unwanted effects occur:
Effect 2 breaks the guarantee this PR adds. The comment in async_polling.py (lines 49–51) says the transport does not close while a request uses it. This condition occurs when an application sets a time limit on shutdown. Examples:
Suggested fixDo not await the task directly. Use asyncio.wait: async def wait_stopped(self):
"""Waits for the task to finish unwinding after ``stop()``. A no-op if
the task never started or is the current task."""
task = self.__task
if task is not None and task is not asyncio.current_task():
# asyncio.wait does not cancel the task and does not raise the
# task's exception. Cancellation of the caller propagates normally.
await asyncio.wait({task})join_handle() in the same module uses this pattern. Its comment gives the reason. TestsThe two tests below show the problem. They assert the correct behavior:
Add the tests to ldclient/testing/impl/datasource/ (as a new file, or move the class into test_async_polling.py): """
Tests demonstrating that AsyncRepeatingTask.wait_stopped() mishandles
cancellation of its *caller*.
`await task` makes the polling task the awaiting coroutine's `_fut_waiter`, so
cancelling the caller of stop() (an ``asyncio.wait_for`` deadline, an
``asyncio.timeout`` block, a TaskGroup tearing down) has two effects:
1. the cancellation is forwarded *into* the polling task, aborting whatever
cancellation cleanup it was doing (e.g. a persistent store finishing a
write, aiohttp connection teardown), and
2. the caller's own cancellation is then absorbed by the blanket
``except asyncio.CancelledError``, so stop() keeps going, closes the
transport out from under the still-unwinding poll, and reports success.
Both tests assert the *desired* behavior, so they FAIL on the current code and
pass once wait_stopped() waits without forwarding cancellation, e.g.::
async def wait_stopped(self):
task = self.__task
if task is not None and task is not asyncio.current_task():
await asyncio.wait({task})
(the same pattern join_handle() in this module already uses, for the reason
its comment explains).
"""
import asyncio
from unittest.mock import AsyncMock, MagicMock
import pytest
from ldclient.testing.impl.datasource.test_async_polling import make_processor
class TestStopUnderExternalCancellation:
@pytest.mark.asyncio
async def test_stop_under_deadline_reports_timeout_and_does_not_abandon_cleanup(self):
# An application shutting down under a deadline:
# await asyncio.wait_for(client.close(), timeout=...)
# If the in-flight poll's cancellation cleanup outlives the deadline,
# the caller must see TimeoutError; the cleanup must not be aborted,
# and the transport must not be closed under the live poll.
events = []
started = asyncio.Event()
async def slow_poll():
started.set()
try:
await asyncio.sleep(60)
except asyncio.CancelledError:
events.append('cleanup_started')
# Simulates a store commit / connection teardown that takes
# longer than the shutdown deadline below.
await asyncio.sleep(0.3)
events.append('cleanup_finished')
raise
async def close():
events.append('transport_closed')
requester = MagicMock()
requester.get_all_data = slow_poll
requester.close = close
processor = make_processor(requester=requester)
processor.start()
await asyncio.wait_for(started.wait(), timeout=1.0)
# Desired: the missed deadline is reported. Today wait_for() returns
# normally, because wait_stopped() swallows the CancelledError that
# wait_for delivers to stop().
with pytest.raises(asyncio.TimeoutError):
await asyncio.wait_for(processor.stop(), timeout=0.1)
# Give the polling task time to finish unwinding on its own.
await asyncio.sleep(0.4)
# Desired: the cleanup ran to completion instead of being aborted by
# the forwarded cancellation...
assert events[:2] == ['cleanup_started', 'cleanup_finished']
# ...and the transport was never closed while the poll was live.
if 'transport_closed' in events:
assert events.index('transport_closed') > events.index('cleanup_finished')
@pytest.mark.asyncio
async def test_cancelling_a_task_blocked_in_stop_actually_cancels_it(self):
# A TaskGroup sibling failure or lifespan teardown cancels the task
# that is running stop(). Desired: that task ends cancelled, and the
# cancellation is not forwarded into the polling task's cleanup.
cleanup_completed = asyncio.Event()
started = asyncio.Event()
async def slow_poll():
started.set()
try:
await asyncio.sleep(60)
except asyncio.CancelledError:
await asyncio.sleep(0.2)
cleanup_completed.set()
raise
requester = MagicMock()
requester.get_all_data = slow_poll
requester.close = AsyncMock()
processor = make_processor(requester=requester)
processor.start()
await asyncio.wait_for(started.wait(), timeout=1.0)
shutdown = asyncio.ensure_future(processor.stop())
await asyncio.sleep(0.05) # shutdown is now blocked inside wait_stopped()
shutdown.cancel()
# Desired: the cancellation propagates. Today stop() swallows it and
# returns normally, so the application's cancellation is lost.
with pytest.raises(asyncio.CancelledError):
await shutdown
assert shutdown.cancelled()
# Desired: the polling task's cleanup still ran to completion.
await asyncio.wait_for(cleanup_completed.wait(), timeout=1.0)Note: with the fix, stop() can now stop before await self._requester.close() when its caller cancels it. A try/finally around lines 52–53 of async_polling.py makes sure the transport closes in that path too. |
Sorry, something went wrong.
Drop the async feature requester's duplicate endpoint definition; use the shared constant from datasource_common instead.
- Don't set _ready on a generic poll exception, so a transient error during startup no longer ends start_wait early (matches sync). - Close the owned HTTP transport on stop: the feature requester tracks whether it created the transport and exposes close(); the polling processor awaits it. - Drop the dead 'all_data is not None' guard (the requester returns cached data on 304, never None) and the fictional None-return polling test.
AsyncRepeatingTask gains wait_stopped() to await the cancelled task; the polling processor's stop() now waits for the in-flight poll to unwind before closing the requester's transport, so awaiting stop() guarantees background work has stopped and the transport isn't closed under a live request.
Addresses review findings on the async FDv1 polling data source: - Drop the store.initialized gate on the VALID status update so async polling reports VALID on every successful poll like the sync data source, instead of getting stuck in INITIALIZING when a store's initialized flag is a false/cached read. - Add an AsyncFeatureRequester interface and implement it, replacing the subclass of the stale sync FeatureRequester ABC (whose get_all method the impl never provided). Keeps get_all_data, which matches the sync implementation, and types the processor's requester param against the interface. - Minor: use plain truthiness in initialized(), spec the test sink as AsyncDataSourceUpdateSink, and simplify the make_config docstring.
AsyncPollingUpdateProcessor.stop awaits requester.close(), and the requester param is typed against AsyncFeatureRequester, so the interface must declare close() — otherwise a substitute requester implementing only the interface would fail at shutdown.
…pped
wait_stopped awaited the worker with a bare await + except CancelledError, which conflated the worker's expected stop() cancellation with cancellation of the caller itself — a timed/cancelled stop() could swallow the cancel and return as if it completed. Use asyncio.wait({task}) (as join_handle already does): it absorbs the worker's cancellation without re-raising it, while still propagating a cancellation of the caller.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 357fbcf. Configure here.
Sorry, something went wrong.
…not leak it AsyncPollingUpdateProcessor.stop() awaited wait_stopped() and only then closed the requester. If the caller of stop() was cancelled during that wait, close() never ran and an owned aiohttp transport could leak. Move the close into a finally so it still runs on cancellation, while letting CancelledError propagate. Adds a regression test that cancels stop() mid-wait and asserts the transport is still closed.
🤖 I have created a release *beep* *boop* --- ## [9.17.0](9.16.1...9.17.0) (2026-08-28) ### Features * Add async big segment store manager and async Redis adapter ([#462](#462)) ([aa492d2](aa492d2)) * Add async DynamoDB persistent feature store ([#490](#490)) ([cb010df](cb010df)) * Add async event processor ([ec7c113](ec7c113)) * Add async event processor ([#472](#472)) ([ec7c113](ec7c113)) * Add async FDv1 polling data source and feature requester ([#475](#475)) ([cca37a8](cca37a8)) * Add async FDv1 streaming and data source status tracking ([#464](#464)) ([4bf7067](4bf7067)) * Add async FDv2 data sources ([#485](#485)) ([5da1515](5da1515)) * Add async FDv2 data system ([#486](#486)) ([6a70132](6a70132)) * Add async hook, plugin, and flag tracker ([#463](#463)) ([686a70a](686a70a)) * Add async migration support ([#470](#470)) ([577d51e](577d51e)) * Add async persistent feature store foundation and Redis adapter ([f9c76ee](f9c76ee)) * Add AsyncConfig for the async SDK client ([#471](#471)) ([0587a78](0587a78)) * Add AsyncLDClient with FDv1 data system and public API ([#480](#480)) ([fd041a5](fd041a5)) * Add Config.with_wrapper_information ([#501](#501)) ([8a98583](8a98583)) * Add environment ID support for hooks. ([#484](#484)) ([49e809f](49e809f)) * Add read-only store views and async persistence foundation for the data system ([#503](#503)) ([0eb61fa](0eb61fa)) ### Bug Fixes * Allow tombstones without a key property ([#502](#502)) ([5f44e61](5f44e61)) * Escape attribute names reported in redactedAttributes ([#505](#505)) ([90059cb](90059cb)) * Prevent a persistent-store outage from throwing in the sync FDv2 evaluation ([#506](#506)) ([467da53](467da53)) * Return empty prerequisites for a flag that fails to evaluate in all_flags_state ([#483](#483)) ([73e9b07](73e9b07)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Release Please** bumps the package from **9.16.1** to **9.17.0** in `pyproject.toml`, `ldclient/version.py`, `.release-please-manifest.json`, and the provenance example in `PROVENANCE.md`. > > `CHANGELOG.md` gains a new **9.17.0** (2026-08-28) section that records what ships in this minor release: a broad **async** surface (`AsyncLDClient`, `AsyncConfig`, async FDv1/FDv2 data systems, event processor, hooks/plugins, migration, and Redis/DynamoDB persistent stores plus big-segment async support), plus sync improvements (`Config.with_wrapper_information`, hook environment ID, read-only store views) and bug fixes (tombstones, `redactedAttributes` escaping, FDv2 persistent-store resilience, `all_flags_state` prerequisites). > > No application logic changes appear in this diff—only version metadata and release notes. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f225e46. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| Back | FazBrowse Home | New Git URL |
Overview
PR 7 of the SDK-60 async epic: the async FDv1 polling data source and feature requester.
Stacking
This PR is stacked on #464 (base branch jb/sdk-2743/async-fdv1-streaming), which provides the shared datasource_common module these files import. Until #464 merges, this PR will also show #464's commits in its diff; a rebase after #464 merges will drop them, leaving only the three files here.
SDK-2825
Note
Medium Risk
New experimental async flag-ingestion path affects client initialization and data-source status; shutdown and HTTP error handling must stay correct to avoid leaks or stuck waits.
Overview
Adds experimental async FDv1 polling: an AsyncFeatureRequester contract plus AsyncFeatureRequesterImpl that GETs the poll endpoint with gzip, optional payload filter query param, and ETag / 304 caching before returning flags and segments.
AsyncPollingUpdateProcessor runs polls on AsyncRepeatingTask, writes via sink_or_store, sets ready when the store initializes, and reports VALID / INTERRUPTED / OFF on success, recoverable errors, and fatal HTTP failures (matching sync polling semantics, including unblocking init on unrecoverable errors).
AsyncRepeatingTask.wait_stopped() lets shutdown wait for the in-flight poll to finish; stop() on the processor uses that (with requester.close() in finally) so transports are not closed mid-request.
Broad unit tests cover caching, transport ownership, error recovery, sink status, and shutdown ordering.
Reviewed by Cursor Bugbot for commit 305f2b0. Bugbot is set up for automated code reviews on this repo. Configure here.