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.