diff --git a/LIBEV_SHUTDOWN_ANALYSIS.md b/LIBEV_SHUTDOWN_ANALYSIS.md new file mode 100644 index 0000000000..dbbaa7d715 --- /dev/null +++ b/LIBEV_SHUTDOWN_ANALYSIS.md @@ -0,0 +1,317 @@ +# LibevWrapper Shutdown Crash Analysis + +## Executive Summary + +The `libevreactor.py` module uses `atexit.register(partial(_cleanup, _global_loop))` to clean up the event loop during Python shutdown. However, this is registered when `_global_loop = None`, causing the cleanup to receive `None` instead of the actual loop instance. This prevents proper shutdown and can lead to crashes when libev callbacks execute during Python interpreter shutdown. + +## Root Cause Analysis + +### The Bug + +In `cassandra/io/libevreactor.py` lines 230-231: + +```python +_global_loop = None +atexit.register(partial(_cleanup, _global_loop)) +``` + +The problem: +1. `_global_loop` is `None` at module import time +2. `partial(_cleanup, _global_loop)` captures `None` as the first argument +3. Later, `LibevConnection.initialize_reactor()` sets `_global_loop` to a `LibevLoop` instance +4. During shutdown, atexit calls `_cleanup(None)` instead of `_cleanup()` +5. The `_cleanup` function checks `if loop:` and returns immediately without doing anything + +### Why This Causes Crashes + +When cleanup doesn't run: + +1. **Event loop thread keeps running**: The loop thread (`_run_loop`) continues executing +2. **Watchers remain active**: IO, Timer, and Prepare watchers are not stopped +3. **Python objects may be deallocated**: During shutdown, Python starts tearing down modules +4. **Callbacks can fire after Python teardown**: The C callbacks in `libevwrapper.c` can be triggered: + - `io_callback()` - for socket I/O events + - `timer_callback()` - for timer events + - `prepare_callback()` - before each event loop iteration + +5. **Crash scenarios**: + - Callbacks call `PyGILState_Ensure()` when Python may be finalizing + - Callbacks call `PyObject_CallFunction()` on potentially deallocated objects + - Callbacks access `self->callback` which may point to freed memory + - `PyErr_WriteUnraisable()` may fail if the error handling system is torn down + +## Additional Crash Scenarios + +### 1. Race Condition: Thread Join Timeout + +The cleanup code has a 1-second timeout for joining the event loop thread: + +```python +def _cleanup(self): + # ... + with self._lock_thread: + self._thread.join(timeout=1.0) + + if self._thread.is_alive(): + log.warning("Event loop thread could not be joined...") +``` + +**Crash scenario**: If the thread doesn't join in time, it continues running while Python tears down, accessing deallocated objects. + +### 2. GIL State Issues During Finalization + +All C callbacks use `PyGILState_Ensure()` / `PyGILState_Release()`: + +```c +static void io_callback(struct ev_loop *loop, ev_io *watcher, int revents) { + libevwrapper_IO *self = watcher->data; + PyObject *result; + PyGILState_STATE gstate = PyGILState_Ensure(); // May fail during shutdown + // ... + PyGILState_Release(gstate); +} +``` + +**Crash scenario**: During interpreter shutdown, the GIL state management may be invalid or cause deadlocks. + +### 3. Object Lifecycle Issues + +Watchers hold references to Python callbacks: + +```c +typedef struct libevwrapper_IO { + PyObject_HEAD + struct ev_io io; + struct libevwrapper_Loop *loop; + PyObject *callback; // This may be deallocated during shutdown +} libevwrapper_IO; +``` + +**Crash scenario**: +- Python starts deallocating objects during shutdown +- The event loop is still running and fires a callback +- The callback tries to call `self->callback` which points to freed memory +- Segmentation fault + +### 4. Connection Cleanup Not Triggered + +Without proper cleanup, connections are not closed: + +```python +def _cleanup(self): + # This never runs if loop is None! + for conn in self._live_conns | self._new_conns | self._closed_conns: + conn.close() + for watcher in (conn._write_watcher, conn._read_watcher): + if watcher: + watcher.stop() +``` + +**Crash scenario**: Active connections with pending I/O can trigger callbacks after Python shutdown. + +### 5. Module Deallocation Order + +Python doesn't guarantee module deallocation order during shutdown. The libev loop might try to access: +- The `logging` module (for `log.debug()`, `log.warning()`) +- The `os` module (for PID checks) +- The `threading` module (for locks and threads) +- The `time` module (for timers) + +**Crash scenario**: If these modules are deallocated before libev callbacks finish, accessing them causes crashes. + +### 6. Fork Handling Issues + +The code has fork detection: + +```python +if _global_loop._pid != os.getpid(): + log.debug("Detected fork, clearing and reinitializing reactor state") + cls.handle_fork() + _global_loop = LibevLoop() +``` + +**Crash scenario**: In a forked child process, if atexit cleanup runs, it might try to clean up the parent's loop state, causing issues. + +## Why This Affects Scylla/Cassandra Tests Specifically + +From the issue comments: + +> "in our tests that it happens, the Cassandra/scylla server is still up, when we shutdown the interpreter" +> "we have in flight request" + +**Test scenario that triggers the bug**: + +1. Test creates cluster connection +2. Test sends queries (creates active watchers and callbacks) +3. Test completes but server is still responding +4. Test framework exits Python interpreter +5. Active connections have pending I/O events +6. Atexit runs but does nothing (receives None) +7. Event loop keeps running, server sends response +8. IO callback fires during Python shutdown → CRASH + +## Impact Analysis + +### Frequency +- **Intermittent**: Only crashes when timing is "just right" +- **More likely when**: + - Many active connections + - High query rate + - Fast test execution (less time for graceful shutdown) + - Server has pending responses at exit time + +### Severity +- **High**: Causes Python interpreter crashes +- **Hard to debug**: Stack traces may be incomplete or corrupted +- **No workaround**: Clearing all atexit hooks breaks other functionality + +## Proposed Solutions + +### Solution 1: Fix atexit Registration (Minimal Change - RECOMMENDED) + +Replace the problematic line with a wrapper function: + +```python +def _atexit_cleanup(): + """Cleanup function called by atexit that uses the current _global_loop value.""" + global _global_loop + if _global_loop is not None: + _cleanup(_global_loop) + +_global_loop = None +atexit.register(_atexit_cleanup) # Looks up current value at shutdown +``` + +**Pros**: +- Minimal code change (6-7 lines) +- Fixes the immediate bug +- No API changes +- No C extension changes needed + +**Cons**: +- Still relies on atexit (but that's the requirement) + +### Solution 2: Add Loop Stop Method (from issue description) + +Implement the C code suggested in the issue to add a `loop.stop()` method that can break the event loop from any thread: + +```c +typedef struct libevwrapper_Loop { + PyObject_HEAD + struct ev_loop *loop; + ev_async async_watcher; // New field +} libevwrapper_Loop; + +static void async_stop_cb(EV_P_ ev_async *w, int revents) { + ev_break(EV_A_ EVBREAK_ALL); +} + +static PyObject * +Loop_stop(libevwrapper_Loop *self, PyObject *args) { + ev_async_send(self->loop, &self->async_watcher); + Py_RETURN_NONE; +} +``` + +Then in Python: + +```python +def _atexit_cleanup(): + global _global_loop + if _global_loop is not None: + if _global_loop._loop: + _global_loop._loop.stop() # Break the event loop + _cleanup(_global_loop) +``` + +**Pros**: +- Provides explicit loop stopping mechanism +- Thread-safe (async is designed for cross-thread communication) +- More robust cleanup + +**Cons**: +- Requires C extension changes +- More complex change +- Needs thorough testing + +### Solution 3: Callback Safety Guards + +Add safety checks in C callbacks to detect shutdown: + +```c +static void io_callback(struct ev_loop *loop, ev_io *watcher, int revents) { + libevwrapper_IO *self = watcher->data; + PyObject *result; + + // Check if Python is finalizing + if (Py_IsInitialized() == 0) { + return; // Don't execute callbacks during shutdown + } + + PyGILState_STATE gstate = PyGILState_Ensure(); + // ... rest of callback +} +``` + +**Pros**: +- Prevents crashes from callbacks during shutdown +- Defense in depth + +**Cons**: +- Doesn't fix the root cause +- `Py_IsInitialized()` may not catch all shutdown states +- Still need to fix the atexit issue + +### Solution 4: Weakref-based Cleanup (like twistedreactor) + +Similar to `twistedreactor.py`, use weak references: + +```python +def _cleanup(loop_ref): + loop = loop_ref() if callable(loop_ref) else loop_ref + if loop: + loop._cleanup() + +_global_loop = None + +def _register_cleanup(): + global _global_loop + if _global_loop is not None: + import weakref + atexit.register(partial(_cleanup, weakref.ref(_global_loop))) +``` + +**Pros**: +- Better memory management +- Follows existing pattern in codebase + +**Cons**: +- More complex +- Requires calling _register_cleanup() at the right time + +## Recommendation + +**Implement Solution 1 first** (fix atexit) as it: +- Addresses the immediate bug +- Requires minimal changes +- Can be quickly tested and deployed +- Doesn't change any APIs + +**Then consider Solution 2** (add loop.stop()) as an enhancement for more robust shutdown. + +**Optionally add Solution 3** (callback guards) for defense in depth. + +## Testing Strategy + +1. **Unit tests**: Verify atexit callback captures correct loop instance +2. **Subprocess tests**: Verify cleanup runs correctly at process exit +3. **Integration tests**: Test with active connections and pending I/O +4. **Stress tests**: Many connections, rapid creation/destruction +5. **Fork tests**: Verify behavior in forked processes + +## References + +- GitHub Issue: scylladb/scylla-cluster-tests#11713 +- GitHub Issue: scylladb/scylladb#17564 +- Existing test: `test_watchers_are_finished` in `test_libevreactor.py` +- Related code: `twistedreactor.py` (uses weakref approach) diff --git a/tests/unit/io/test_libevreactor_shutdown.py b/tests/unit/io/test_libevreactor_shutdown.py new file mode 100644 index 0000000000..6be2c2b647 --- /dev/null +++ b/tests/unit/io/test_libevreactor_shutdown.py @@ -0,0 +1,250 @@ +# Copyright DataStax, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +Test to demonstrate the libevwrapper atexit cleanup issue. + +This test demonstrates the problem where the atexit callback is registered +with _global_loop=None at import time, causing it to receive None during +shutdown instead of the actual loop instance. +""" + +import unittest +import atexit +import sys +import subprocess +import tempfile +import os +from pathlib import Path + +from cassandra import DependencyException + +try: + from cassandra.io.libevreactor import LibevConnection +except (ImportError, DependencyException): + LibevConnection = None + +from tests import is_monkey_patched + + +class LibevAtexitCleanupTest(unittest.TestCase): + """ + Test case to demonstrate the atexit cleanup bug in libevreactor. + + The bug: atexit.register(partial(_cleanup, _global_loop)) is called when + _global_loop is None, so the cleanup function receives None at shutdown + instead of the actual LibevLoop instance that was created later. + """ + + def setUp(self): + if is_monkey_patched(): + raise unittest.SkipTest("Can't test libev with monkey patching") + if LibevConnection is None: + raise unittest.SkipTest('libev does not appear to be installed correctly') + + def test_atexit_callback_registered_with_none(self): + """ + Test that demonstrates the atexit callback bug. + + The atexit.register(partial(_cleanup, _global_loop)) line is executed + when _global_loop is None. This means the partial function captures + None as the argument, and when atexit calls it during shutdown, it + passes None to _cleanup instead of the actual loop instance. + + @since 3.29 + @jira_ticket PYTHON-XXX + @expected_result The test demonstrates that atexit cleanup is broken + + @test_category connection + """ + from cassandra.io import libevreactor + from functools import partial + + # Check the current atexit handlers + # Note: atexit._exithandlers is an implementation detail but useful for debugging + if hasattr(atexit, '_exithandlers'): + # Find our cleanup handler + cleanup_handler = None + for handler in atexit._exithandlers: + func = handler[0] + # Check if this is our partial(_cleanup, _global_loop) handler + if isinstance(func, partial): + if func.func.__name__ == '_cleanup': + cleanup_handler = func + break + + if cleanup_handler: + # The problem: the partial was created with _global_loop=None + # So even if _global_loop is later set to a LibevLoop instance, + # the atexit callback will still call _cleanup(None) + captured_arg = cleanup_handler.args[0] if cleanup_handler.args else None + + # This assertion will fail after LibevConnection.initialize_reactor() + # is called and _global_loop is set to a LibevLoop instance + LibevConnection.initialize_reactor() + + # At this point, libevreactor._global_loop is not None + self.assertIsNotNone(libevreactor._global_loop, + "Global loop should be initialized") + + # But the atexit handler still has None captured! + self.assertIsNone(captured_arg, + "The atexit handler captured None, not the actual loop instance. " + "This is the BUG: cleanup will receive None at shutdown!") + + def test_shutdown_crash_scenario_subprocess(self): + """ + Test that simulates a Python shutdown crash scenario in a subprocess. + + This test creates a minimal script that: + 1. Imports the driver + 2. Creates a connection (which starts the event loop) + 3. Exits without explicit cleanup + + The expected behavior is that atexit should clean up the loop, but + because of the bug, the cleanup receives None and doesn't actually + stop the loop or its watchers. This can lead to crashes if callbacks + fire during shutdown. + + @since 3.29 + @jira_ticket PYTHON-XXX + @expected_result The subprocess demonstrates the cleanup issue + + @test_category connection + """ + # Create a test script that demonstrates the issue + test_script = ''' +import sys +import os + +# Add the driver path +sys.path.insert(0, {driver_path!r}) + +# Import and setup +from cassandra.io.libevreactor import LibevConnection, _global_loop +import atexit + +# Initialize the reactor (creates the global loop) +LibevConnection.initialize_reactor() + +print("Global loop initialized:", _global_loop is not None) + +# Check what atexit will actually call +if hasattr(atexit, '_exithandlers'): + from functools import partial + for handler in atexit._exithandlers: + func = handler[0] + if isinstance(func, partial) and func.func.__name__ == '_cleanup': + captured_arg = func.args[0] if func.args else None + print("Atexit will call _cleanup with:", captured_arg) + print("But _global_loop is:", _global_loop) + print("BUG: Cleanup will receive None instead of the loop!") + break + +# Exit without explicit cleanup - atexit should handle it, but won't! +print("Exiting...") +''' + + driver_path = str(Path(__file__).parent.parent.parent.parent) + script_content = test_script.format(driver_path=driver_path) + + with tempfile.NamedTemporaryFile(mode='w', suffix='.py', delete=False) as f: + f.write(script_content) + script_path = f.name + + try: + result = subprocess.run( + [sys.executable, script_path], + capture_output=True, + text=True, + timeout=5 + ) + + output = result.stdout + print("\n=== Subprocess Output ===") + print(output) + print("=== End Output ===\n") + + # Verify the output shows the bug + self.assertIn("Global loop initialized: True", output) + self.assertIn("Atexit will call _cleanup with: None", output) + self.assertIn("BUG: Cleanup will receive None instead of the loop!", output) + + finally: + os.unlink(script_path) + + +class LibevShutdownRaceConditionTest(unittest.TestCase): + """ + Tests to analyze potential race conditions and crashes during shutdown. + """ + + def setUp(self): + if is_monkey_patched(): + raise unittest.SkipTest("Can't test libev with monkey patching") + if LibevConnection is None: + raise unittest.SkipTest('libev does not appear to be installed correctly') + + def test_callback_during_shutdown_scenario(self): + """ + Test to document the potential crash scenario. + + When Python is shutting down: + 1. Various modules are being torn down + 2. The libev event loop may still be running + 3. If a callback (io_callback, timer_callback, prepare_callback) fires: + - It calls PyGILState_Ensure() + - It tries to call Python functions (PyObject_CallFunction) + - If Python objects have been deallocated, this can crash + + The root cause: The atexit cleanup doesn't actually run because it + receives None instead of the loop instance, so it never: + - Sets _shutdown flag + - Stops watchers + - Joins the event loop thread + + @since 3.29 + @jira_ticket PYTHON-XXX + @expected_result Documents the crash scenario + + @test_category connection + """ + from cassandra.io.libevreactor import _global_loop, _cleanup + + # This test documents the issue - we can't easily reproduce a crash + # in a unit test without actually tearing down Python, but we can + # verify the conditions that lead to it + + LibevConnection.initialize_reactor() + + # Verify the loop exists + self.assertIsNotNone(_global_loop) + + # Simulate what atexit would call (with the bug) + _cleanup(None) # BUG: receives None instead of _global_loop + + # The loop is still running because cleanup did nothing! + self.assertFalse(_global_loop._shutdown, + "Loop should NOT be shut down when cleanup receives None") + + # Now call it correctly + _cleanup(_global_loop) + + # Now it should be shut down + self.assertTrue(_global_loop._shutdown, + "Loop should be shut down when cleanup receives the actual loop") + + +if __name__ == '__main__': + unittest.main()