Schedule HTTP/2 re-enable on NetVConnection owner thread

[saimayithri]

Refs: #13358

Summary

Http2CommonSession::state_complete_frame_read() schedules
HTTP2_SESSION_EVENT_REENABLE on the thread currently holding the session
mutex.

That thread can differ from the underlying NetVConnection owner thread.
When the re-enable handler later processes frames or releases streams, it can
reach UnixNetVConnection queue operations from the wrong thread and trigger
the NetHandler lock assertion described in #13358.

This change schedules the re-enable event on the session NetVConnection's
owner thread instead.

Changes

At both HTTP/2 re-enable scheduling sites:

• Keeps the existing continuation, delay, event type, and VIO unchanged.
• Uses get_netvc()->thread as the scheduling target instead of
  get_mutex()->thread_holding.
• Asserts that the session has an associated NetVConnection before
  scheduling.

Validation

• Built successfully with:
  cmake --build build-dev -j4

• Ran:
  ctest --test-dir build-dev -j4

Scope

This PR addresses the HTTP/2 re-enable path from #13358 only. The separate
HTTP/1.1 cache-write teardown path remains under investigation.

[JosiahWI]

The code looks good! Thank you for this fix!

[JosiahWI]

I'm adding this to the v10.2.x project in anticipation that it should be backported.

[masaori335]

[approve ci]

[JosiahWI]

The AuTest subcookie spuriously failed, and the HostDBProcessor regression test failed (unsure if this is spurious).

[saimayithri]

Thanks for the review, assignment, and for running CI !!

[JosiahWI]

Approving, but would be good to also get a review from @wuxcer.

[JosiahWI]

The feedback from @wuxcer (#13358 (comment)) reveals that the NetVConnection thread isn't always the NetHandler thread. Could we schedule on nh->thread instead of netvc->thread?

[saimayithri]

Hi! I spent some time tracing this further, and I think I may have been focusing on the wrong comparison.

From the current source, I couldn't find a path where netvc->thread and nh->thread diverge. They appear to be assigned together during initialization, and I couldn't find any later reassignment.

However, @wuxcer's latest crash report seems to indicate something different: the mitigation check if (this->thread != this_ethread()) had already passed before MUTEX_TRY_LOCK(nh->mutex) failed. That made me wonder if the important distinction is actually between vc->thread and the thread currently holding nh->mutex, rather than vc->thread and nh->thread.

When you mentioned that the NetVConnection thread isn't always the NetHandler thread, I just wanted to make sure I'm interpreting that correctly. Were you referring to nh->thread, or to the thread currently holding nh->mutex? I feel like I'm missing an important piece of the locking model, and I'd like to understand that before updating the scheduling target.

[JosiahWI]

Thank you for correcting my confusion, @saimayithri. As long as the netvc thread and the nh thread are the same, it is OK to schedule the queue change on the netvc thread. The only concern, then, is that that same thread must be holding the nh mutex when it calls the NetHandler method to add to the queue, or the TRY_LOCK will still fail. Do you know if this patch also fixes that?

[saimayithri]

Thanks! I don't think I've completely proven that yet.

From what I've traced so far, my change ensures the reenable event is scheduled onto the NetVConnection/NetHandler owner thread, but I haven't yet verified whether that also guarantees nh->mutex is already held (or otherwise guarantees the MUTEX_TRY_LOCK succeeds) by the time execution reaches add_to_active_queue(). Given @wuxcer's latest report, that seems to be the remaining question.

I'll trace that path next and verify whether scheduling onto the owner thread is sufficient to satisfy the MUTEX_TRY_LOCK invariant, or whether there's another execution path where that invariant can still be violated.

[moonchen]

I just wanted to further clarify state_complete_frame_read and do_process_frame_read should only be called from ET_NET threads. The correct fix would to identify why these were called on the wrong thread, and fix that.

[saimayithri]

Thanks for the clarification! I’ll look into why these are being called from the wrong thread and investigate the root cause.

[saimayithri]

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?

[JosiahWI]

@saimayithri I am so sorry, you did a lot of research on this and I missed it somehow. Thank you for the thorough trace. I am not knowledgeable enough to give you the green light on adding the logic for the thread switching to terminate_if_possible right now, but I will make sure to bring this PR back into the review cycle in the community meeting today.