|
| 1 | +# Implementation Summary: LibevWrapper Cleanup Fix |
| 2 | + |
| 3 | +## Overview |
| 4 | + |
| 5 | +This document summarizes the implementation of fixes for the libevwrapper atexit cleanup bug that caused Python shutdown crashes. |
| 6 | + |
| 7 | +## Problem Statement |
| 8 | + |
| 9 | +The libev reactor was using `atexit.register(partial(_cleanup, _global_loop))` where `_global_loop` was `None` at module import time. This caused the cleanup function to receive `None` at shutdown instead of the actual loop instance, preventing proper cleanup and leading to crashes when callbacks fired during Python interpreter shutdown. |
| 10 | + |
| 11 | +## Solutions Implemented |
| 12 | + |
| 13 | +### Solution 1: Fix atexit Registration (Commit: 8c90f05) |
| 14 | + |
| 15 | +**Minimal change approach - fixes the root cause** |
| 16 | + |
| 17 | +#### Changes Made: |
| 18 | +1. Replaced `atexit.register(partial(_cleanup, _global_loop))` with a wrapper function |
| 19 | +2. Created `_atexit_cleanup()` that looks up `_global_loop` when called, not when registered |
| 20 | +3. Removed unused `partial` import from functools |
| 21 | +4. Updated tests to verify the fix works |
| 22 | + |
| 23 | +#### Files Modified: |
| 24 | +- `cassandra/io/libevreactor.py` - Added `_atexit_cleanup()` wrapper function |
| 25 | +- `tests/unit/io/test_libevreactor_shutdown.py` - Updated tests to verify fix |
| 26 | + |
| 27 | +#### Code Changes: |
| 28 | +```python |
| 29 | +# OLD (buggy): |
| 30 | +_global_loop = None |
| 31 | +atexit.register(partial(_cleanup, _global_loop)) # Captures None! |
| 32 | + |
| 33 | +# NEW (fixed): |
| 34 | +def _atexit_cleanup(): |
| 35 | + """Cleanup function that looks up _global_loop at shutdown time.""" |
| 36 | + global _global_loop |
| 37 | + if _global_loop is not None: |
| 38 | + _cleanup(_global_loop) |
| 39 | + |
| 40 | +_global_loop = None |
| 41 | +atexit.register(_atexit_cleanup) # Looks up current value when called |
| 42 | +``` |
| 43 | + |
| 44 | +#### Benefits: |
| 45 | +- Minimal change (14 lines modified) |
| 46 | +- Fixes the immediate bug |
| 47 | +- No C extension changes required |
| 48 | +- No API changes |
| 49 | +- Easy to understand and maintain |
| 50 | + |
| 51 | +### Solution 2: Add loop.stop() Method (Commit: 466e4cb) |
| 52 | + |
| 53 | +**Enhanced robustness - adds explicit loop stopping mechanism** |
| 54 | + |
| 55 | +#### Changes Made: |
| 56 | +1. Added `ev_async async_watcher` field to `libevwrapper_Loop` struct |
| 57 | +2. Implemented `async_stop_cb()` callback that calls `ev_break(EVBREAK_ALL)` |
| 58 | +3. Added `Loop_stop()` Python-callable method |
| 59 | +4. Initialize and start async_watcher in `Loop_init()` |
| 60 | +5. Clean up async_watcher in `Loop_dealloc()` |
| 61 | +6. Updated `_atexit_cleanup()` to call `loop.stop()` before cleanup |
| 62 | + |
| 63 | +#### Files Modified: |
| 64 | +- `cassandra/io/libevwrapper.c` - Added stop method to C extension |
| 65 | +- `cassandra/io/libevreactor.py` - Call loop.stop() in cleanup |
| 66 | + |
| 67 | +#### Code Changes: |
| 68 | + |
| 69 | +**C Extension (libevwrapper.c):** |
| 70 | +```c |
| 71 | +typedef struct libevwrapper_Loop { |
| 72 | + PyObject_HEAD |
| 73 | + struct ev_loop *loop; |
| 74 | + ev_async async_watcher; // NEW: for thread-safe stopping |
| 75 | +} libevwrapper_Loop; |
| 76 | + |
| 77 | +static void async_stop_cb(EV_P_ ev_async *w, int revents) { |
| 78 | + ev_break(EV_A_ EVBREAK_ALL); // Break the event loop |
| 79 | +} |
| 80 | + |
| 81 | +static PyObject * |
| 82 | +Loop_stop(libevwrapper_Loop *self, PyObject *args) { |
| 83 | + ev_async_send(self->loop, &self->async_watcher); |
| 84 | + Py_RETURN_NONE; |
| 85 | +} |
| 86 | + |
| 87 | +// In Loop_init: |
| 88 | +ev_async_init(&self->async_watcher, async_stop_cb); |
| 89 | +ev_async_start(self->loop, &self->async_watcher); |
| 90 | +``` |
| 91 | +
|
| 92 | +**Python (libevreactor.py):** |
| 93 | +```python |
| 94 | +def _atexit_cleanup(): |
| 95 | + global _global_loop |
| 96 | + if _global_loop is not None: |
| 97 | + # Stop the event loop before cleanup (thread-safe) |
| 98 | + if _global_loop._loop: |
| 99 | + try: |
| 100 | + _global_loop._loop.stop() |
| 101 | + except Exception: |
| 102 | + pass # Continue cleanup even if stop fails |
| 103 | + _cleanup(_global_loop) |
| 104 | +``` |
| 105 | + |
| 106 | +#### Benefits: |
| 107 | +- Thread-safe loop stopping via libev's async mechanism |
| 108 | +- Explicitly breaks event loop before cleanup |
| 109 | +- Prevents callbacks from firing during cleanup |
| 110 | +- Works with Solution 1 for defense in depth |
| 111 | +- Implements the approach suggested in the original issue |
| 112 | + |
| 113 | +## Testing |
| 114 | + |
| 115 | +### Tests Updated: |
| 116 | +1. `test_atexit_callback_uses_current_global_loop()` - Verifies atexit handler is the wrapper function, not a partial |
| 117 | +2. `test_shutdown_cleanup_works_with_fix()` - Subprocess test verifying proper cleanup |
| 118 | +3. `test_cleanup_with_fix_properly_shuts_down()` - Verifies cleanup actually shuts down the loop |
| 119 | + |
| 120 | +### Test Results: |
| 121 | +Tests verify that: |
| 122 | +- The atexit handler is `_atexit_cleanup`, not a `partial` object |
| 123 | +- `_global_loop` is properly looked up at shutdown time |
| 124 | +- Cleanup receives the actual loop instance, not `None` |
| 125 | +- The loop is properly shut down after cleanup runs |
| 126 | + |
| 127 | +## Impact Analysis |
| 128 | + |
| 129 | +### Before the Fix: |
| 130 | +- atexit cleanup received `None` and did nothing |
| 131 | +- Event loop kept running during Python shutdown |
| 132 | +- Callbacks could fire after Python started deallocating modules |
| 133 | +- Resulted in segmentation faults and crashes |
| 134 | + |
| 135 | +### After Solution 1: |
| 136 | +- atexit cleanup receives actual loop instance |
| 137 | +- Proper shutdown sequence executes: |
| 138 | + - Sets `_shutdown` flag |
| 139 | + - Closes connections |
| 140 | + - Stops watchers |
| 141 | + - Joins event loop thread (with 1s timeout) |
| 142 | + |
| 143 | +### After Solution 2 (Additional): |
| 144 | +- Event loop explicitly breaks before cleanup starts |
| 145 | +- Prevents race conditions where loop is processing events during cleanup |
| 146 | +- Thread-safe stopping via libev's async watcher mechanism |
| 147 | +- More robust cleanup even if loop is in the middle of event processing |
| 148 | + |
| 149 | +## Deployment Considerations |
| 150 | + |
| 151 | +### Building: |
| 152 | +Solution 2 requires rebuilding the C extension: |
| 153 | +```bash |
| 154 | +python setup.py build_ext --inplace |
| 155 | +``` |
| 156 | + |
| 157 | +### Compatibility: |
| 158 | +- Both solutions maintain backward compatibility |
| 159 | +- No API changes for users |
| 160 | +- The `stop()` method is a new addition (doesn't break existing code) |
| 161 | + |
| 162 | +### Testing Recommendations: |
| 163 | +1. Test with active connections during shutdown |
| 164 | +2. Test with pending I/O operations |
| 165 | +3. Test in forked processes |
| 166 | +4. Stress test with many connections |
| 167 | +5. Test rapid creation/destruction cycles |
| 168 | + |
| 169 | +## Crash Scenarios Addressed |
| 170 | + |
| 171 | +1. ✅ **Root cause fixed**: Cleanup now receives actual loop instance |
| 172 | +2. ✅ **Thread join timeout**: Loop is stopped before trying to join thread |
| 173 | +3. ✅ **Object lifecycle**: Watchers are properly stopped before Python teardown |
| 174 | +4. ✅ **Connection cleanup**: Cleanup actually runs, closing connections and stopping watchers |
| 175 | +5. ✅ **Race conditions**: Explicit loop.stop() prevents callbacks during cleanup |
| 176 | +6. ⚠️ **GIL state issues**: Still possible but less likely (could be addressed with Solution 3) |
| 177 | +7. ⚠️ **Module deallocation order**: Python still controls this, but shorter cleanup window reduces risk |
| 178 | +8. ⚠️ **Fork handling**: Existing fork detection should work with the fix |
| 179 | + |
| 180 | +## Future Enhancements (Optional) |
| 181 | + |
| 182 | +### Solution 3: Callback Safety Guards |
| 183 | +Could add additional safety in C callbacks: |
| 184 | +```c |
| 185 | +static void io_callback(...) { |
| 186 | + if (Py_IsInitialized() == 0) { |
| 187 | + return; // Don't execute during shutdown |
| 188 | + } |
| 189 | + // ... rest of callback |
| 190 | +} |
| 191 | +``` |
| 192 | +
|
| 193 | +This would provide defense in depth but isn't strictly necessary with Solutions 1 and 2 in place. |
| 194 | +
|
| 195 | +## Conclusion |
| 196 | +
|
| 197 | +Both solutions have been implemented: |
| 198 | +- **Solution 1** fixes the root cause with minimal changes |
| 199 | +- **Solution 2** adds robustness with explicit loop stopping |
| 200 | +
|
| 201 | +Together, they provide a comprehensive fix for the libev shutdown crash issue. |
0 commit comments