cmcfarlen commented on PR #13487: URL: https://github.com/apache/trafficserver/pull/13487#issuecomment-5196081268
The analysis holds up against `PendingAction`, and the fix looks right. `PendingAction::operator=` cancels whatever it holds on *any* assignment, including `nullptr` (`include/tscore/PendingAction.h:99-114` — `ACTION_RESULT_DONE` is the only exemption). So `pending_action = _ua.get_txn()->adjust_thread(...)` killed the captive `HttpCacheSM` action on **both** paths, not just the common one: - `adjust_thread()` returns `nullptr` (already on the right thread): assigns `nullptr`, cancels the captive action, falls through to the rest of the handler with a cancelled action still owned by `HttpCacheSM`. - `adjust_thread()` returns a reschedule event: *also* cancels the captive action, then returns early — so it is dead before the retry even runs. Hoisting the clear fixes both. It is safe to hoist because `clear_if_action_is` is a `compare_exchange_strong` to `nullptr` with no cancel (`PendingAction.h:137-147`), and the action has already called back by the time this handler runs. It also matches the ordering `state_cache_open_read` (`HttpSM.cc:2663`) and `state_http_server_open` (`HttpSM.cc:1851`) already use. @JosiahWI raises a good point that I missed on my first read — this patch makes **two** changes, and only the first is described. It adds the clear before the assignment *and removes* the one after it. Having worked the removal through, I believe it is safe, and since it isn't observable under normal thread conditions it seems worth stating explicitly: After `pending_action = adjust_thread(...)`, `pending_action` can only hold `nullptr` or a **freshly scheduled** reschedule event — never `data` itself. So the old post-assignment `clear_if_action_is(data)` could never match in that position and was already dead code there. The `if (_ua.get_txn())` false path is covered too, since the hoisted clear now runs unconditionally ahead of it, and the unconditional `pending_action = nullptr` a few lines later handles the remaining teardown. Clearing both before and after, as @JosiahWI suggests, would be harmless but redundant. One question on the autest, not a blocker: it leans on a `delay: 3s` origin response overlapping a `delay: 200ms` client request to lose the write lock, which is a real timing window rather than a deterministic one. How did it hold up over repeated CI runs, and is 3s comfortable on a loaded builder? The good news is that it fails loudly rather than silently passing if the window ever closes — the `contains` assertions on `falling back to read retry` (`HttpCacheSM.cc:288`) and `READ_RETRY cache read failed, bypassing cache` (`HttpTransact.cc:3528`) would catch it. That's the right design; I'd just like to know how it behaved across runs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
