Fix critical race conditions and signal safety issues - #1228
Open
benroeder wants to merge 6 commits into
Open
Conversation
Root cause: The stream redirector could raise "ValueError: fd X added twice" when there was a state mismatch between its internal _active dictionary and the Tornado IOLoop's registered handlers. This occurred during rapid process respawning or when exceptions left the redirector's state inconsistent. The fix adds defensive programming to _start_one() and _stop_one(): - Before adding a handler, attempt to remove any existing one first - Improve cleanup to handle both IOLoop and internal state consistently - Maintain state consistency even if operations partially fail This prevents the ValueError and allows the redirector to recover gracefully from state mismatches that can occur due to race conditions or improper cleanup during process lifecycle events. Added regression test that reproduces the exact error condition from production and verifies the fix prevents it.
Root cause: The synchronized decorator prevents concurrent execution of arbiter_start_watchers. During arbiter initialization, start_watchers() is called. If a client sends a 'start' command at that exact moment, it receives ConflictError because the command is already running. This fix makes start_watchers check if watchers are already running (using watcher.is_stopped()) before attempting to start them. Watchers that are already running are skipped, making the operation idempotent. Impact: - Reduces ConflictError occurrences in production environments - Makes concurrent start commands more resilient - No breaking changes - this is purely defensive programming The error typically occurs when monitoring systems or automated scripts send start commands while the arbiter is still initializing or when all watchers are already running.
Root cause: In manage_watchers(), when handling on-demand watchers, the code was calling _start_watchers() without yielding it. Since _start_watchers is a coroutine decorated with @gen.coroutine, this meant the coroutine was not being executed properly - it would just return a Future object instead of actually starting the watchers. This fix adds the missing yield statement to ensure _start_watchers executes correctly when triggered by socket activity for on-demand watchers. Impact: - On-demand watchers now start correctly when socket activity occurs - Eliminates potential race conditions from unyielded coroutines - No breaking changes Note: The ConflictError between manage_watchers and watcher_stop seen in production logs is expected behavior - the synchronized decorator prevents these operations from running concurrently.
After testing IOLoop behavior, discovered that: - remove_handler() never throws exceptions, even for non-existent handlers - The try/except blocks in _stop_one were unnecessary - The error handling in _start_one was overly complex This refactor simplifies the code by: - Removing unnecessary exception handling in _stop_one - Simplifying _start_one to just call remove_handler before add_handler - Removing unreachable code paths The fix still prevents the 'fd added twice' error but with cleaner, more maintainable code. Added tests to verify IOLoop behavior and ensure the simplified approach works correctly.
Root cause: Signal handlers were performing unsafe operations including logging, dictionary lookups, and string formatting. These operations can cause deadlocks if a signal interrupts code holding locks that the signal handler tries to acquire, or crashes from memory corruption. The fix moves all unsafe operations from signal context to the main thread using Tornado's add_callback_from_signal(). The signal handler now only performs the single safe operation of scheduling a callback. Changes: - Signal handler only calls add_callback_from_signal() - All logging, lookups, and processing moved to _handle_signal_in_main_thread() - quit() and reload() methods updated since they now run in main thread - Added emergency fallback using only write() and _exit() if callback fails Impact: - Prevents deadlocks from signal handlers trying to acquire locks - Eliminates undefined behavior from non-async-signal-safe operations - Makes signal handling robust under high load conditions This is a critical fix for production stability.
The test now verifies that our signal handler implementation is safe, rather than demonstrating the old unsafe behavior.
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
1 participant
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.
Summary
Test plan
These fixes address critical stability issues that were causing production failures, particularly around signal handling deadlocks and race
conditions during process management.