Repository navigation
fix(aio): keep host_monitoring_v2 monitors per event loop - #1289
Closed
AhmadMasry wants to merge 1 commit into
Closed
AhmadMasry wants to merge 1 commit into
AhmadMasry wants to merge 1 commit into
Conversation
Every monitored statement ran _cleanup_idle_monitors, which stopped any monitor that wasn't is_usable(), and is_usable() was False whenever the monitor ran on a different loop from the caller's. With one event loop per thread, each loop stopped the other loops' monitors on every statement: monitors and their monitoring connections were recreated continuously, and in-flight statements on the other loop lost failure detection. Key the registry by monitor key and event loop, so each loop reuses its own monitor, and treat a monitor as unusable only when it is stopped, its task has finished, or its loop is closed. Guard the registry with a lock, as sync's monitor service does. Fixes aws#1287
Contributor
Author
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.
Description
Fixes #1287.
With the async API,
host_monitoring_v2monitors were stopped and recreated on every statement when an application runs more than one asyncio event loop (for example one per worker thread):_cleanup_idle_monitors(), which stops monitors that aren'tis_usable();AsyncHostMonitorV2.is_usable()returnedFalsewhenever the monitor ran on a different loop from the caller's;Each recreation opened a new monitoring connection, and statements still running on the other loop lost failure detection.
Changes in
aio/host_monitoring_plugin.py:(monitor key, event loop). Each loop gets one monitor per host and detection settings and keeps reusing it. One per loop is needed because the monitor is an asyncio task and aborts connections that belong to its loop.is_usable()isFalseonly when the monitor is stopped, its task has finished, or its loop is closed. Idle expiry is unchanged, and stopping another loop's idle monitor already goes throughcancel_task_threadsafe.RLock.Compared with sync, which shares one monitor per settings and host across all threads, an application with N event loops now holds N monitoring connections per monitored host, instead of a new one per statement.
Testing:
tests/unit/test_aio_host_monitoring_plugin.py:test_event_loops_do_not_stop_each_others_monitors: statements alternating between a loop on another thread and the test's loop keep exactly two monitors, none stopped. Failed before the fix (one monitor left).test_monitor_whose_loop_closed_is_replaced: a monitor whose loop has closed is still stopped and replaced.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.