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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to