Skip to content

fix(events): prevent deadlock when closing or waiting for listeners from within a listener - #2088

Merged
vdusek merged 8 commits into
masterfrom
fix/event-manager-listener-self-await
Aug 5, 2026
Merged

fix(events): prevent deadlock when closing or waiting for listeners from within a listener#2088
vdusek merged 8 commits into
masterfrom
fix/event-manager-listener-self-await

Conversation

@vdusek

@vdusek vdusek commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Calling EventManager.wait_for_all_listeners_to_complete() - or closing the manager via __aexit__ - from within an event listener deadlocked. The listener runs in a task that is itself registered in _listener_tasks, so the wait ends up awaiting the very task that is awaiting it. Under a close timeout this cycle degrades further into a RecursionError.

Changes in EventManager:

  • Listener tasks currently blocked in wait_for_all_listeners_to_complete() are tracked in _waiting_listener_tasks and excluded from the wait, so listener waiters never await themselves or each other. A caller that is not a listener is outside the cycle and still awaits every listener, waiting ones included.
  • The wait no longer wraps the gather in an inner task. The one-tick defer that task provided is now explicit: emit only schedules the listener wrappers, and each registers its listener task once it starts running, so the wait yields before snapshotting _listener_tasks. This also drops the Event listener raised an exception. log line, which duplicated the ERROR the listener wrapper already logs with the traceback.
  • The listener wrapper's finally uses set.discard() instead of set.remove(), since __aexit__ may have already cleared the task set while the listener was mid-flight (avoids a spurious KeyError).

Regression tests cover waiting from within a listener, several listeners waiting at once, closing the manager from within a listener, and waiting from outside while a listener is itself waiting.

This unblocks apify/apify-sdk-python#1061, where Actor.exit() is called from inside an event listener (e.g. an ABORTING handler) - with this fix the SDK can drop its _detach_current_listener_task workaround.

✍️ Drafted by Claude Code

@vdusek vdusek added t-tooling Issues with this label are in the ownership of the tooling team. adhoc Ad-hoc unplanned task added during the sprint. labels Jul 21, 2026
@vdusek vdusek self-assigned this Jul 21, 2026
@github-actions github-actions Bot added this to the 145th sprint - Tooling team milestone Jul 21, 2026
@github-actions github-actions Bot added the tested Temporary label used only programatically for some analytics. label Jul 21, 2026
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.57%. Comparing base (fae204b) to head (10ed7b5).
⚠️ Report is 20 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2088   +/-   ##
=======================================
  Coverage   93.57%   93.57%           
=======================================
  Files         181      181           
  Lines       12590    12644   +54     
=======================================
+ Hits        11781    11832   +51     
- Misses        809      812    +3     
Flag Coverage Δ
unit 93.57% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread src/crawlee/events/_event_manager.py Outdated

@Mantisus Mantisus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few nits

Comment thread tests/unit/events/test_event_manager.py Outdated
Comment thread tests/unit/events/test_event_manager.py Outdated
@vdusek
vdusek removed the request for review from janbuchar August 5, 2026 08:08
@vdusek
vdusek requested a review from B4nan August 5, 2026 08:23
@vdusek
vdusek merged commit 92ab97a into master Aug 5, 2026
36 checks passed
@vdusek
vdusek deleted the fix/event-manager-listener-self-await branch August 5, 2026 10:04
vdusek added a commit to apify/apify-sdk-python that referenced this pull request Aug 5, 2026
…ee lock

crawlee's EventManager now handles this deadlock upstream (apify/crawlee-python#2088),
so the SDK-side workaround is redundant. The lockfile is bumped to crawlee 1.9.1b4,
which contains the fix; the regression test passes without any SDK-side code changes.
The declared crawlee constraint in pyproject.toml stays >=1.8.0,<2.0.0 until crawlee
ships a stable release with the fix.
vdusek added a commit that referenced this pull request Aug 6, 2026
…lease (#2100)

Get rid of `pyee` as we really don't need it and modernize the
`EventManager` and `LocalEventManager`. No public API changes.

### Fixes

- A listener whose `__call__` is async (a class instance, not a
function) was treated as sync and handed to `asyncio.to_thread`, which
returned its coroutine without ever awaiting it - the listener silently
never ran.
- `off()` decided between "remove this listener" and "remove all
listeners of the event" by the truthiness of `listener`, so a listener
object that is falsy in a boolean context wiped all listeners of the
event.
- `off()` left an empty entry behind in `_listeners_to_wrappers`,
keeping a reference to a listener that is no longer registered, and
created entries for events and listeners that were never registered at
all.
- A listener that fits neither call shape (it takes two parameters, say)
raised out of its own task instead of being logged like any other
failing listener. Whether the listener takes the event data is now
resolved once at registration, so the invocation itself is fully covered
by the logging.
- `__aexit__` released the context only on its happy path. A
cancellation or a failing emission left the manager active for good, and
since re-entering an active manager is a no-op, `PersistState` was never
emitted again.

### pyee is gone

`emit` creates the listener task itself instead of going through
`pyee.asyncio.AsyncIOEventEmitter`, which:

- halves the tasks per invocation - the emitter used to schedule a
wrapper task that spawned and awaited a second, inner listener task,
- registers every listener task synchronously in `emit`, so
`wait_for_all_listeners_to_complete()` can no longer miss the listeners
of a just-emitted event - the one-tick defer that #2088 had to make
explicit is not needed anymore,
- drops a runtime dependency (`pyee` is still pulled in by `playwright`
for the browser extras).

The `_listeners_to_wrappers` layout is deliberately kept as it is -
apify-sdk-python reaches into it in `Actor.reboot()`. Its full unit test
suite passes against this branch.

### The rest

- Everything that depends only on the listener (whether it takes the
event data, sync/async, its name) is resolved once at registration
instead of on every invocation, and the wrapping moved out of `on()`
into `_wrap_listener`.
- The two `__aexit__` branches were collapsed into a single `last_exit`
flag, `__aenter__` mirrors it, and the manual "not active" check was
replaced by the `ensure_context` decorator, so the message matches the
one every other method raises.
- The three `EventManager.on.listener_wrapper(): ...` DEBUG lines are
gone - they reported that a task is awaited, that it completed, and that
it was discarded.
- `LocalEventManager` reads the CPU and the memory info concurrently -
`get_cpu_info` alone blocks its thread for 100 ms while sampling the
CPU.
- Typing: `__aenter__ -> Self` and `set[asyncio.Task[None]]`.
- `wait_for_all_tasks_for_finish` renamed to
`wait_for_all_tasks_to_finish`.
- The listener helpers are private static methods of `EventManager`, and
the attributes of both managers are documented with docstrings instead
of comments.

New tests cover every fix above, the synchronous task registration in
`emit`, the nested context teardown, and the concurrent system info
readings.

*✍️ Drafted by Claude Code*
vdusek added a commit to apify/apify-sdk-python that referenced this pull request Aug 7, 2026
…ers (#1061)

`Actor.exit()` / `Actor.fail()` called from within an event listener
used to deadlock into a `RecursionError`, since the cleanup path waited
on the very listener task that called it.

Fixed upstream in crawlee's `EventManager`:
[apify/crawlee-python#2088](apify/crawlee-python#2088)
(merged). On Python 3.11 it still deadlocks, because `asyncio.wait_for`
there wraps the awaited coroutine in a separate task, which defeats
crawlee's self-wait detection. This is a minor edge case, so we're not
adding an SDK-side workaround just for Python 3.11.

This PR:
- Adds a regression test for `Actor.exit()` called from an `ABORTING`
listener, skipped on Python 3.11.
- Bumps `uv.lock` (not the declared `pyproject.toml` constraint) to
`crawlee==1.9.1b4`, so CI exercises the fix.

Follow-up: bump the declared `crawlee` constraint in `pyproject.toml`
once a stable release with the fix ships.

*✍️ Drafted by Claude Code*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants