saimayithri commented on issue #13358:
URL:
https://github.com/apache/trafficserver/issues/13358#issuecomment-4976518797
Thanks again for continuing to test this in production. I spent some more
time tracing the current source to understand the locking model before making
any further changes to the patch.
Here's what I've been able to establish from the current source:
* `netvc->thread` and `nh->thread` appear to be assigned together during
NetHandler/NetVConnection initialization, and I couldn't find any later
reassignment that would cause them to diverge.
* `Http1ClientSession`, `Http2ClientSession`, and `Http2ConnectionState` are
all initialized with a `nullptr` continuation mutex, and during
`new_connection()` they are assigned `new_vc->mutex`. In other words, these
continuations execute under `vc->mutex`, not `nh->mutex`.
* `EThread::process_event()` acquires only the continuation mutex before
invoking `handleEvent()`. It does not automatically acquire `nh->mutex`.
* `NetHandler::waitForActivity()` acquires `nh->mutex` while processing the
network poll and ready list, and from tracing `EThread::execute_regular()`,
queued events (`process_event()`) and `waitForActivity()` execute in separate
phases of the event loop.
From that, my current understanding is that queue operations like
`add_to_active_queue()` are not expected to inherit `nh->mutex` from the
continuation. Instead, they appear to rely on `MUTEX_TRY_LOCK(nh->mutex)`
acquiring it on the owner thread.
However, your latest production crash exposed a contradiction that I haven't
been able to explain from the source alone.
The owner-thread check appears to have already passed:
```cpp
if (this->thread != this_ethread()) {
...
}
```
yet immediately afterwards:
```cpp
MUTEX_TRY_LOCK(lock, nh->mutex, this_ethread());
```
failed.
If my understanding of the execution model is correct, I would have expected
that try-lock to succeed when executing on the owner thread. Since your
production result contradicts that expectation, I don't think I can honestly
claim that scheduling `HTTP2_SESSION_EVENT_REENABLE` onto `netvc->thread` is
sufficient without understanding what is actually holding `nh->mutex` at that
point.
Would you be willing to run one build with a little diagnostic
instrumentation immediately before the assertion? I think that would tell us
exactly which invariant is being violated.
For example:
```cpp
Warning("add_to_active_queue try-lock failed:"
" vc=%p"
" current=%p"
" vc->thread=%p"
" nh->thread=%p"
" holder=%p"
" vc->mutex=%p"
" nh->mutex=%p",
this,
this_ethread(),
this->thread,
nh->thread,
nh->mutex->thread_holding,
this->mutex,
nh->mutex);
```
If possible, it would also be useful to log the current event (or otherwise
confirm whether this is still occurring on the `HTTP2_SESSION_EVENT_REENABLE`
path).
I think this should let us distinguish between a few possibilities:
* the execution isn't actually occurring on the expected owner thread,
* another execution context still owns `nh->mutex`,
* or there's another path reaching `add_to_active_queue()` that I haven't
identified yet.
Once we know who actually owns `nh->mutex` at the point of failure, it
should be much easier to determine whether the scheduling change is
insufficient by itself or whether there's another concurrency path that still
needs to be addressed.
--
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]
