Re: [PR] Schedule HTTP/2 re-enable on NetVConnection owner thread [trafficserver]

["saimayithri (via GitHub)" <gi...@apache.org>]

saimayithri commented on PR #13362:
URL: https://github.com/apache/trafficserver/pull/13362#issuecomment-5535879746

   Hey, I traced the H2 teardown path further against the current checkout and believe I found a concrete ET_NET affinity violation.
   
   The path is:
   
   `CacheProcessor (ET_CACHE) → HttpSM → Http2Stream::do_io_close() → terminate_if_possible() → ~Http2Stream() → Http2ConnectionState::release_stream() → Http2Session::do_clear_session_active() → NetVC::add_to_keep_alive_queue()`
   
   `CacheProcessor` can invoke the HttpSM callback inline on the cache thread. Since `do_io_close()` remains synchronous, `terminate_if_possible()` can therefore reach the sole `THREAD_FREE` path on ET_CACHE. Destruction then reaches `release_stream()`, which can manipulate the session NetVC's NetHandler queue from the wrong thread.
   
   I don't think moving `_switch_thread_if_not_on_right_thread()` to the beginning of `do_io_close()` is safe, because HttpSM teardown relies on the close state being established synchronously before `transaction_done()` completes.
   
   The more appropriate boundary appears to be `terminate_if_possible()` itself: let `do_io_close()` synchronously establish the closed state, then when `terminate_if_possible()` is reached from ET_CACHE, bounce the destruction onto the stream's `_thread` using the existing `_switch_thread_if_not_on_right_thread()` mechanism.
   
   On the ET_NET event, `main_event_handler()` clears `cross_thread_event`, the `VC_EVENT_EOS` case with `nullptr` performs no VIO callback, and control reaches `terminate_if_possible()` again, where `THREAD_FREE` can then occur on the correct thread.
   
   So the minimal change I am considering is to enforce the existing thread-affinity check immediately before the existing destruction block in `terminate_if_possible()`, rather than weakening the NetHandler queue assertions.
   
   This tracing specifically explains the H2 teardown path; I'm treating the H2 `create_stream()` / `add_to_active_queue()` crash separately.
   
   Could you confirm whether `terminate_if_possible()` is the appropriate boundary for this fix before I implement it?
   


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org