Skip to content

Commit 2ea75f5

Browse files
committed
fix: Release BoundedTaskSet slots synchronously to prevent a livelock
BoundedTaskSet freed a slot in a done-callback, which runs on a later loop turn. When flush tasks finished and a flush_and_wait or shutdown message arrived in the same event-loop batch, the dispatcher's retry loop (while not self._trigger_flush(): await self._flush_workers.wait()) could spin forever: try_run still counted the finished tasks, and wait() over already-complete tasks did not yield, so the done-callbacks never ran to free the slots. This wedged the event loop at ~100% CPU. Release the slot from within the task, synchronously in a finally, like the sync FixedThreadPool. Add regression tests at the BoundedTaskSet and event processor levels, and drop an unused import.
1 parent 1dfcdfd commit 2ea75f5

4 files changed

Lines changed: 128 additions & 7 deletions

File tree

‎ldclient/impl/aio/concurrency.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -267,15 +267,19 @@ def try_run(self, job: Callable[[], Coroutine]) -> bool:
267267
task was started, or False if the set is full or has been stopped."""
268268
if not self._accepting or len(self._tasks) >= self._limit:
269269
return False
270-
task = asyncio.create_task(job())
270+
task = asyncio.create_task(self._run(job))
271271
self._tasks.add(task)
272-
task.add_done_callback(self._on_done)
273272
return True
274273

275-
def _on_done(self, task: asyncio.Task) -> None:
276-
self._tasks.discard(task)
277-
if not task.cancelled() and task.exception() is not None:
278-
log.warning('Unhandled exception in background task', exc_info=task.exception())
274+
async def _run(self, job: Callable[[], Coroutine]) -> None:
275+
try:
276+
await job()
277+
except Exception:
278+
log.warning('Unhandled exception in background task', exc_info=True)
279+
finally:
280+
task = asyncio.current_task()
281+
if task is not None:
282+
self._tasks.discard(task)
279283

280284
async def wait(self) -> None:
281285
"""Waits for the tasks currently running to complete. This does not stop

‎ldclient/impl/events/async_event_processor.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
import gzip
77
import json
88
import queue
9-
import time
109
import uuid
1110
from collections import namedtuple
1211
from random import Random

‎ldclient/testing/impl/events/test_async_event_processor.py‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import asyncio
1111
import gzip
1212
import json
13+
import threading
1314
import uuid
1415
from contextlib import asynccontextmanager
1516
from email.utils import formatdate
@@ -491,3 +492,88 @@ async def test_event_payload_is_gzip_compressed_when_enabled():
491492
output = json.loads(gzip.decompress(mock_http.request_data))
492493
assert len(output) == 1
493494
assert output[0]['kind'] == 'identify'
495+
496+
497+
async def test_flush_and_wait_completes_when_in_flight_posts_finish_together():
498+
# Regression test for a livelock: when all in-flight flush POSTs finish and
499+
# the flush_and_wait message arrives in the same event-loop batch, the
500+
# dispatcher's retry loop must still make progress and reply, not spin.
501+
class GatedMockAioHttp(MockAioHttp):
502+
"""Parks each request until the gate is set, then completes without yielding."""
503+
504+
def __init__(self):
505+
super().__init__()
506+
self.gate = asyncio.Event()
507+
508+
def request(self, method, uri, headers=None, data=None, timeout=None, proxy=None):
509+
self._recorded_requests.append((headers, data))
510+
outer = self
511+
512+
class _Ctx:
513+
async def __aenter__(self):
514+
await outer.gate.wait()
515+
return MockAioResponse(200, {})
516+
517+
async def __aexit__(self, exc_type, exc_value, traceback):
518+
return False
519+
520+
return _Ctx()
521+
522+
# DefaultAsyncEventProcessor discards the dispatcher, so capture it here to
523+
# let the watchdog recover the loop if it wedges.
524+
dispatcher_holder = []
525+
526+
def capture_dispatcher(inbox, config, http, diagnostic_accumulator):
527+
dispatcher = EventDispatcher(inbox, config, http, diagnostic_accumulator)
528+
dispatcher_holder.append(dispatcher)
529+
return dispatcher
530+
531+
mock_http = GatedMockAioHttp()
532+
config = AsyncConfig(sdk_key='SDK_KEY', diagnostic_opt_out=True)
533+
ep = DefaultAsyncEventProcessor(config, mock_http, dispatcher_class=capture_dispatcher)
534+
try:
535+
# Saturate all five flush workers with parked POSTs.
536+
for i in range(5):
537+
ep.send_event(EventInputIdentify(timestamp, Context.create('user%d' % i)))
538+
ep.flush()
539+
deadline = asyncio.get_running_loop().time() + 2
540+
while len(mock_http.recorded_requests) < 5:
541+
assert asyncio.get_running_loop().time() < deadline, 'workers never saturated'
542+
await asyncio.sleep(0.01)
543+
544+
# Buffer one more event so the awaited flush has work to hand off.
545+
ep.send_event(EventInputIdentify(timestamp, context))
546+
await asyncio.sleep(0.05)
547+
548+
# Complete all in-flight POSTs and enqueue flush_and_wait in one loop
549+
# callback, so the dispatcher sees the message in the batch the workers
550+
# finished in.
551+
reply = AsyncEvent()
552+
mock_http.gate.set()
553+
ep._inbox.put_nowait(EventProcessorMessage('flush_and_wait', reply))
554+
555+
# A wedged loop cannot fire asyncio timeouts, so a watchdog thread
556+
# detects the wedge and frees the task set to fail with an assertion
557+
# rather than hang the run.
558+
finished = threading.Event()
559+
wedged = threading.Event()
560+
561+
def watchdog():
562+
if not finished.wait(3):
563+
wedged.set()
564+
dispatcher_holder[0]._flush_workers._tasks.clear()
565+
566+
threading.Thread(target=watchdog, daemon=True).start()
567+
568+
replied = await reply.wait(10)
569+
finished.set()
570+
571+
assert replied is True
572+
assert not wedged.is_set(), (
573+
"flush_and_wait wedged the event loop: the dispatcher spun in "
574+
"'while not self._trigger_flush(): await self._flush_workers.wait()' "
575+
"without yielding, and only the watchdog un-stuck it"
576+
)
577+
finally:
578+
mock_http.gate.set()
579+
await ep.stop()

‎ldclient/testing/test_aio.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,38 @@ async def test_wait_returns_when_idle(self):
223223
await tasks.wait()
224224
tasks.stop()
225225

226+
@pytest.mark.asyncio
227+
async def test_retry_pattern_makes_progress_when_task_finished_in_same_batch(self):
228+
tasks = aio.BoundedTaskSet(1)
229+
finished = asyncio.Event()
230+
231+
async def job():
232+
finished.set()
233+
234+
assert tasks.try_run(job) is True
235+
236+
# A raw asyncio.Event wakes this coroutine in the same loop batch the
237+
# job's task finished in, before try_run sees the slot freed.
238+
await finished.wait()
239+
240+
async def noop():
241+
pass
242+
243+
# Mirrors the retry loop in EventDispatcher._run_main_loop: try_run must
244+
# free the finished task's slot, or the real (unbounded) loop spins.
245+
accepted = False
246+
for _ in range(100):
247+
if tasks.try_run(noop):
248+
accepted = True
249+
break
250+
await tasks.wait()
251+
252+
assert accepted, (
253+
"try_run never freed capacity: the finished task was still counted "
254+
"against the limit, so the dispatcher's retry loop would spin forever"
255+
)
256+
await tasks.wait()
257+
226258

227259
class TestTaskRunnerParity:
228260
@pytest.mark.asyncio

0 commit comments

Comments
 (0)