|
| 1 | +# LibevWrapper Shutdown Crash Analysis |
| 2 | + |
| 3 | +## Executive Summary |
| 4 | + |
| 5 | +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. |
| 6 | + |
| 7 | +## Root Cause Analysis |
| 8 | + |
| 9 | +### The Bug |
| 10 | + |
| 11 | +In `cassandra/io/libevreactor.py` lines 230-231: |
| 12 | + |
| 13 | +```python |
| 14 | +_global_loop = None |
| 15 | +atexit.register(partial(_cleanup, _global_loop)) |
| 16 | +``` |
| 17 | + |
| 18 | +The problem: |
| 19 | +1. `_global_loop` is `None` at module import time |
| 20 | +2. `partial(_cleanup, _global_loop)` captures `None` as the first argument |
| 21 | +3. Later, `LibevConnection.initialize_reactor()` sets `_global_loop` to a `LibevLoop` instance |
| 22 | +4. During shutdown, atexit calls `_cleanup(None)` instead of `_cleanup(<actual_loop>)` |
| 23 | +5. The `_cleanup` function checks `if loop:` and returns immediately without doing anything |
| 24 | + |
| 25 | +### Why This Causes Crashes |
| 26 | + |
| 27 | +When cleanup doesn't run: |
| 28 | + |
| 29 | +1. **Event loop thread keeps running**: The loop thread (`_run_loop`) continues executing |
| 30 | +2. **Watchers remain active**: IO, Timer, and Prepare watchers are not stopped |
| 31 | +3. **Python objects may be deallocated**: During shutdown, Python starts tearing down modules |
| 32 | +4. **Callbacks can fire after Python teardown**: The C callbacks in `libevwrapper.c` can be triggered: |
| 33 | + - `io_callback()` - for socket I/O events |
| 34 | + - `timer_callback()` - for timer events |
| 35 | + - `prepare_callback()` - before each event loop iteration |
| 36 | + |
| 37 | +5. **Crash scenarios**: |
| 38 | + - Callbacks call `PyGILState_Ensure()` when Python may be finalizing |
| 39 | + - Callbacks call `PyObject_CallFunction()` on potentially deallocated objects |
| 40 | + - Callbacks access `self->callback` which may point to freed memory |
| 41 | + - `PyErr_WriteUnraisable()` may fail if the error handling system is torn down |
| 42 | + |
| 43 | +## Additional Crash Scenarios |
| 44 | + |
| 45 | +### 1. Race Condition: Thread Join Timeout |
| 46 | + |
| 47 | +The cleanup code has a 1-second timeout for joining the event loop thread: |
| 48 | + |
| 49 | +```python |
| 50 | +def _cleanup(self): |
| 51 | + # ... |
| 52 | + with self._lock_thread: |
| 53 | + self._thread.join(timeout=1.0) |
| 54 | + |
| 55 | + if self._thread.is_alive(): |
| 56 | + log.warning("Event loop thread could not be joined...") |
| 57 | +``` |
| 58 | + |
| 59 | +**Crash scenario**: If the thread doesn't join in time, it continues running while Python tears down, accessing deallocated objects. |
| 60 | + |
| 61 | +### 2. GIL State Issues During Finalization |
| 62 | + |
| 63 | +All C callbacks use `PyGILState_Ensure()` / `PyGILState_Release()`: |
| 64 | + |
| 65 | +```c |
| 66 | +static void io_callback(struct ev_loop *loop, ev_io *watcher, int revents) { |
| 67 | + libevwrapper_IO *self = watcher->data; |
| 68 | + PyObject *result; |
| 69 | + PyGILState_STATE gstate = PyGILState_Ensure(); // May fail during shutdown |
| 70 | + // ... |
| 71 | + PyGILState_Release(gstate); |
| 72 | +} |
| 73 | +``` |
| 74 | +
|
| 75 | +**Crash scenario**: During interpreter shutdown, the GIL state management may be invalid or cause deadlocks. |
| 76 | +
|
| 77 | +### 3. Object Lifecycle Issues |
| 78 | +
|
| 79 | +Watchers hold references to Python callbacks: |
| 80 | +
|
| 81 | +```c |
| 82 | +typedef struct libevwrapper_IO { |
| 83 | + PyObject_HEAD |
| 84 | + struct ev_io io; |
| 85 | + struct libevwrapper_Loop *loop; |
| 86 | + PyObject *callback; // This may be deallocated during shutdown |
| 87 | +} libevwrapper_IO; |
| 88 | +``` |
| 89 | + |
| 90 | +**Crash scenario**: |
| 91 | +- Python starts deallocating objects during shutdown |
| 92 | +- The event loop is still running and fires a callback |
| 93 | +- The callback tries to call `self->callback` which points to freed memory |
| 94 | +- Segmentation fault |
| 95 | + |
| 96 | +### 4. Connection Cleanup Not Triggered |
| 97 | + |
| 98 | +Without proper cleanup, connections are not closed: |
| 99 | + |
| 100 | +```python |
| 101 | +def _cleanup(self): |
| 102 | + # This never runs if loop is None! |
| 103 | + for conn in self._live_conns | self._new_conns | self._closed_conns: |
| 104 | + conn.close() |
| 105 | + for watcher in (conn._write_watcher, conn._read_watcher): |
| 106 | + if watcher: |
| 107 | + watcher.stop() |
| 108 | +``` |
| 109 | + |
| 110 | +**Crash scenario**: Active connections with pending I/O can trigger callbacks after Python shutdown. |
| 111 | + |
| 112 | +### 5. Module Deallocation Order |
| 113 | + |
| 114 | +Python doesn't guarantee module deallocation order during shutdown. The libev loop might try to access: |
| 115 | +- The `logging` module (for `log.debug()`, `log.warning()`) |
| 116 | +- The `os` module (for PID checks) |
| 117 | +- The `threading` module (for locks and threads) |
| 118 | +- The `time` module (for timers) |
| 119 | + |
| 120 | +**Crash scenario**: If these modules are deallocated before libev callbacks finish, accessing them causes crashes. |
| 121 | + |
| 122 | +### 6. Fork Handling Issues |
| 123 | + |
| 124 | +The code has fork detection: |
| 125 | + |
| 126 | +```python |
| 127 | +if _global_loop._pid != os.getpid(): |
| 128 | + log.debug("Detected fork, clearing and reinitializing reactor state") |
| 129 | + cls.handle_fork() |
| 130 | + _global_loop = LibevLoop() |
| 131 | +``` |
| 132 | + |
| 133 | +**Crash scenario**: In a forked child process, if atexit cleanup runs, it might try to clean up the parent's loop state, causing issues. |
| 134 | + |
| 135 | +## Why This Affects Scylla/Cassandra Tests Specifically |
| 136 | + |
| 137 | +From the issue comments: |
| 138 | + |
| 139 | +> "in our tests that it happens, the Cassandra/scylla server is still up, when we shutdown the interpreter" |
| 140 | +> "we have in flight request" |
| 141 | +
|
| 142 | +**Test scenario that triggers the bug**: |
| 143 | + |
| 144 | +1. Test creates cluster connection |
| 145 | +2. Test sends queries (creates active watchers and callbacks) |
| 146 | +3. Test completes but server is still responding |
| 147 | +4. Test framework exits Python interpreter |
| 148 | +5. Active connections have pending I/O events |
| 149 | +6. Atexit runs but does nothing (receives None) |
| 150 | +7. Event loop keeps running, server sends response |
| 151 | +8. IO callback fires during Python shutdown → CRASH |
| 152 | + |
| 153 | +## Impact Analysis |
| 154 | + |
| 155 | +### Frequency |
| 156 | +- **Intermittent**: Only crashes when timing is "just right" |
| 157 | +- **More likely when**: |
| 158 | + - Many active connections |
| 159 | + - High query rate |
| 160 | + - Fast test execution (less time for graceful shutdown) |
| 161 | + - Server has pending responses at exit time |
| 162 | + |
| 163 | +### Severity |
| 164 | +- **High**: Causes Python interpreter crashes |
| 165 | +- **Hard to debug**: Stack traces may be incomplete or corrupted |
| 166 | +- **No workaround**: Clearing all atexit hooks breaks other functionality |
| 167 | + |
| 168 | +## Proposed Solutions |
| 169 | + |
| 170 | +### Solution 1: Fix atexit Registration (Minimal Change - RECOMMENDED) |
| 171 | + |
| 172 | +Replace the problematic line with a wrapper function: |
| 173 | + |
| 174 | +```python |
| 175 | +def _atexit_cleanup(): |
| 176 | + """Cleanup function called by atexit that uses the current _global_loop value.""" |
| 177 | + global _global_loop |
| 178 | + if _global_loop is not None: |
| 179 | + _cleanup(_global_loop) |
| 180 | + |
| 181 | +_global_loop = None |
| 182 | +atexit.register(_atexit_cleanup) # Looks up current value at shutdown |
| 183 | +``` |
| 184 | + |
| 185 | +**Pros**: |
| 186 | +- Minimal code change (6-7 lines) |
| 187 | +- Fixes the immediate bug |
| 188 | +- No API changes |
| 189 | +- No C extension changes needed |
| 190 | + |
| 191 | +**Cons**: |
| 192 | +- Still relies on atexit (but that's the requirement) |
| 193 | + |
| 194 | +### Solution 2: Add Loop Stop Method (from issue description) |
| 195 | + |
| 196 | +Implement the C code suggested in the issue to add a `loop.stop()` method that can break the event loop from any thread: |
| 197 | + |
| 198 | +```c |
| 199 | +typedef struct libevwrapper_Loop { |
| 200 | + PyObject_HEAD |
| 201 | + struct ev_loop *loop; |
| 202 | + ev_async async_watcher; // New field |
| 203 | +} libevwrapper_Loop; |
| 204 | + |
| 205 | +static void async_stop_cb(EV_P_ ev_async *w, int revents) { |
| 206 | + ev_break(EV_A_ EVBREAK_ALL); |
| 207 | +} |
| 208 | + |
| 209 | +static PyObject * |
| 210 | +Loop_stop(libevwrapper_Loop *self, PyObject *args) { |
| 211 | + ev_async_send(self->loop, &self->async_watcher); |
| 212 | + Py_RETURN_NONE; |
| 213 | +} |
| 214 | +``` |
| 215 | +
|
| 216 | +Then in Python: |
| 217 | +
|
| 218 | +```python |
| 219 | +def _atexit_cleanup(): |
| 220 | + global _global_loop |
| 221 | + if _global_loop is not None: |
| 222 | + if _global_loop._loop: |
| 223 | + _global_loop._loop.stop() # Break the event loop |
| 224 | + _cleanup(_global_loop) |
| 225 | +``` |
| 226 | + |
| 227 | +**Pros**: |
| 228 | +- Provides explicit loop stopping mechanism |
| 229 | +- Thread-safe (async is designed for cross-thread communication) |
| 230 | +- More robust cleanup |
| 231 | + |
| 232 | +**Cons**: |
| 233 | +- Requires C extension changes |
| 234 | +- More complex change |
| 235 | +- Needs thorough testing |
| 236 | + |
| 237 | +### Solution 3: Callback Safety Guards |
| 238 | + |
| 239 | +Add safety checks in C callbacks to detect shutdown: |
| 240 | + |
| 241 | +```c |
| 242 | +static void io_callback(struct ev_loop *loop, ev_io *watcher, int revents) { |
| 243 | + libevwrapper_IO *self = watcher->data; |
| 244 | + PyObject *result; |
| 245 | + |
| 246 | + // Check if Python is finalizing |
| 247 | + if (Py_IsInitialized() == 0) { |
| 248 | + return; // Don't execute callbacks during shutdown |
| 249 | + } |
| 250 | + |
| 251 | + PyGILState_STATE gstate = PyGILState_Ensure(); |
| 252 | + // ... rest of callback |
| 253 | +} |
| 254 | +``` |
| 255 | +
|
| 256 | +**Pros**: |
| 257 | +- Prevents crashes from callbacks during shutdown |
| 258 | +- Defense in depth |
| 259 | +
|
| 260 | +**Cons**: |
| 261 | +- Doesn't fix the root cause |
| 262 | +- `Py_IsInitialized()` may not catch all shutdown states |
| 263 | +- Still need to fix the atexit issue |
| 264 | +
|
| 265 | +### Solution 4: Weakref-based Cleanup (like twistedreactor) |
| 266 | +
|
| 267 | +Similar to `twistedreactor.py`, use weak references: |
| 268 | +
|
| 269 | +```python |
| 270 | +def _cleanup(loop_ref): |
| 271 | + loop = loop_ref() if callable(loop_ref) else loop_ref |
| 272 | + if loop: |
| 273 | + loop._cleanup() |
| 274 | +
|
| 275 | +_global_loop = None |
| 276 | +
|
| 277 | +def _register_cleanup(): |
| 278 | + global _global_loop |
| 279 | + if _global_loop is not None: |
| 280 | + import weakref |
| 281 | + atexit.register(partial(_cleanup, weakref.ref(_global_loop))) |
| 282 | +``` |
| 283 | + |
| 284 | +**Pros**: |
| 285 | +- Better memory management |
| 286 | +- Follows existing pattern in codebase |
| 287 | + |
| 288 | +**Cons**: |
| 289 | +- More complex |
| 290 | +- Requires calling _register_cleanup() at the right time |
| 291 | + |
| 292 | +## Recommendation |
| 293 | + |
| 294 | +**Implement Solution 1 first** (fix atexit) as it: |
| 295 | +- Addresses the immediate bug |
| 296 | +- Requires minimal changes |
| 297 | +- Can be quickly tested and deployed |
| 298 | +- Doesn't change any APIs |
| 299 | + |
| 300 | +**Then consider Solution 2** (add loop.stop()) as an enhancement for more robust shutdown. |
| 301 | + |
| 302 | +**Optionally add Solution 3** (callback guards) for defense in depth. |
| 303 | + |
| 304 | +## Testing Strategy |
| 305 | + |
| 306 | +1. **Unit tests**: Verify atexit callback captures correct loop instance |
| 307 | +2. **Subprocess tests**: Verify cleanup runs correctly at process exit |
| 308 | +3. **Integration tests**: Test with active connections and pending I/O |
| 309 | +4. **Stress tests**: Many connections, rapid creation/destruction |
| 310 | +5. **Fork tests**: Verify behavior in forked processes |
| 311 | + |
| 312 | +## References |
| 313 | + |
| 314 | +- GitHub Issue: scylladb/scylla-cluster-tests#11713 |
| 315 | +- GitHub Issue: scylladb/scylladb#17564 |
| 316 | +- Existing test: `test_watchers_are_finished` in `test_libevreactor.py` |
| 317 | +- Related code: `twistedreactor.py` (uses weakref approach) |
0 commit comments