Skip to content

Commit fb8f5fb

Browse files
authored
chore: fix polling event watcher self-unsubscribe deadlock (#33732)
## Summary & Motivation Closes #5127. Updated the polling watcher to stop invoking callbacks while holding _callback_fn_list_lock, which removes the self-unsubscribe deadlock path in python_modules/dagster/dagster/_core/storage/event_log/polling_event_watcher.py. The implementation now snapshots the callback list under lock and dispatches outside it. I also re-enabled the skipped regression in python_modules/dagster/dagster_tests/storage_tests/utils/event_log_storage.py. ## Test Plan - python -m pytest python_modules/dagster/dagster_tests/storage_tests/test_polling_event_watcher.py - python -m pytest python_modules/dagster/dagster_tests/storage_tests/test_event_log.py -k 'watch_unwatch and TestSqliteEventLogStorage' - python -m pytest python_modules/dagster/dagster_tests/storage_tests/test_event_log.py -k 'watch_unwatch and TestConsolidatedSqliteEventLogStorage' ## Changelog > The changelog is generated by an agent that examines merged PRs and > summarizes/categorizes user-facing changes. You can optionally replace > this text with a terse description of any user-facing changes in your PR, > which the agent will prioritize. Otherwise, delete this section.
1 parent b8b7f3f commit fb8f5fb

2 files changed

Lines changed: 14 additions & 12 deletions

File tree

python_modules/dagster/dagster/_core/storage/event_log/polling_event_watcher.py

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,10 @@ def remove_callback(self, callback: Callable[[EventLogEntry, str], None]):
157157
if not self._callback_fn_list:
158158
self._should_thread_exit.set()
159159

160+
def get_callbacks(self) -> list[CallbackAfterCursor]:
161+
with self._callback_fn_list_lock:
162+
return list(self._callback_fn_list)
163+
160164
def run(self) -> None:
161165
"""Polling function to update Observers with EventLogEntrys from Event Log DB.
162166
Wakes every POLLING_CADENCE &
@@ -177,15 +181,14 @@ def run(self) -> None:
177181
)
178182
cursor = conn.cursor
179183
for event_record in conn.records:
180-
with self._callback_fn_list_lock:
181-
for callback_with_cursor in self._callback_fn_list:
182-
if (
183-
callback_with_cursor.cursor is None
184-
or EventLogCursor.parse(callback_with_cursor.cursor).storage_id()
185-
< event_record.storage_id
186-
):
187-
callback_with_cursor.callback(
188-
event_record.event_log_entry,
189-
str(EventLogCursor.from_storage_id(event_record.storage_id)),
190-
)
184+
for callback_with_cursor in self.get_callbacks():
185+
if (
186+
callback_with_cursor.cursor is None
187+
or EventLogCursor.parse(callback_with_cursor.cursor).storage_id()
188+
< event_record.storage_id
189+
):
190+
callback_with_cursor.callback(
191+
event_record.event_log_entry,
192+
str(EventLogCursor.from_storage_id(event_record.storage_id)),
193+
)
191194
wait_time = INIT_POLL_PERIOD if conn.records else min(wait_time * 2, MAX_POLL_PERIOD)

python_modules/dagster/dagster_tests/storage_tests/utils/event_log_storage.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2710,7 +2710,6 @@ def _throw(_x, _y):
27102710
assert all([isinstance(event, dg.EventLogEntry) for event in event_list])
27112711

27122712
# https://github.com/dagster-io/dagster/issues/5127
2713-
@pytest.mark.skip
27142713
def test_watch_unwatch(self, storage):
27152714
if not self.can_watch():
27162715
pytest.skip("storage cannot watch runs")

0 commit comments

Comments
 (0)