fix: interrupt monitoring buffer wait on shutdown#3457
Open
Sanjays2402 wants to merge 1 commit into
Open
Conversation
Monitoring shutdown could block for a fraction of the configured flush interval because the worker used uninterruptible sleeps. Use an event-backed wait so stop wakes the worker immediately, with a focused regression test.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK monitoring buffer’s background flush worker so shutdown can interrupt the worker’s wait immediately (instead of waiting for a sleep slice), and adds a regression test to ensure stop() completes quickly even with a long flush_interval.
Changes:
- Replace the flush loop’s sliced
time.sleep()waiting with an interruptiblethreading.Event.wait(timeout=...). - Ensure the stop signal wakes the flush thread immediately on shutdown.
- Add a regression test validating
stop()returns quickly and the flush thread terminates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/nexent/monitor/monitoring.py | Adds a stop event and uses it to make the flush loop’s wait interruptible during shutdown. |
| test/sdk/monitor/test_monitoring_buffer.py | Adds a regression test asserting stop() is not delayed by flush_interval-based waits. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
2542
to
+2546
| except Exception as e: | ||
| logger.error(f"Error in monitoring flush loop: {e}") | ||
|
|
||
| for _ in range(10): | ||
| if not self._running: | ||
| return | ||
| time.sleep(self._flush_interval / 10) | ||
| if self._stop_event.wait(timeout=self._flush_interval): | ||
| return |
Comment on lines
+1
to
+15
| import importlib.util | ||
| from pathlib import Path | ||
| import time | ||
|
|
||
|
|
||
| MODULE_PATH = Path(__file__).parents[3] / "sdk" / "nexent" / "monitor" / "monitoring.py" | ||
|
|
||
|
|
||
| def _load_monitoring_module(): | ||
| spec = importlib.util.spec_from_file_location("monitoring_under_test", MODULE_PATH) | ||
| assert spec is not None | ||
| assert spec.loader is not None | ||
| module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(module) | ||
| return module |
| started = time.monotonic() | ||
| buffer.stop() | ||
|
|
||
| assert time.monotonic() - started < 0.5 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Monitoring shutdown could block for up to one-tenth of the configured flush interval because the worker slept in uninterruptible slices. The worker now waits on a stop event, so shutdown wakes it immediately while retaining periodic flushing.
The regression test takes about 2 seconds and fails on the original code, then passes in under 0.5 seconds with this change. Focused pytest, Ruff, compilation, and both diff gates pass; the source file's three existing import-order findings are unchanged from
develop.Closes #3403