Fix potential stall on HTTP/2 connection

[masaori335]

In Http2Stream.cc, below (bit simplified) code appear 5 times. However, this doesn't work as expected when the try lock is not locked.

MUTEX_TRY_LOCK(lock, read_vio.mutex, this_ethread());
if (lock.is_locked()) {
    read_vio.cont->handleEvent(event, &read_vio);
} else {
    this_ethread()->schedule_imm(read_vio.cont, event, &read_vio);
}

The third argument of schedule_imm() is void *cookie and it is assigned to Event::cookie. When the event is processed, the second argument of handleEvent() is Event *.
OTOH, the event handler of the read_vio.cont (HttpTunnel or HttpSM) assume the second void * is VIO *.
This means if we want to pass VIO *, we need to call handleEvent() directly.

trafficserver/proxy/http/HttpTunnel.cc

Line 1559 in baf9d4b

if ((p = get_producer(static_cast<VIO *>(data))) != nullptr) {

trafficserver/proxy/http/HttpSM.cc

Line 2629 in baf9d4b

vc_entry = vc_table.find_entry(static_cast<VIO *>(data));

When ATS falls into the case, the event is ignored and the stream might be stalled.

[maskit]

I understand what problem you are trying to solve, but I'm not sure we should solve it in this way. Doesn't it affect performance? Maybe it doesn't matter because the mechanism is broken already though.

I'd keep the asynchronous mechanism and find a way to pass data correctly.

[shinrich]

Very interesting. I've looked at this code many times, but it never hit me that the scheduled handle event is passing in the event rather than the event->cookie.

It would be good to figure this out in general to avoid deadlocking. There are doubtless other places in the code making this same flawed assumption. Perhaps the scheduled handleEvent call should just always pass in the e->cookie. Of course would need to do a deeper analysis of the code to determine the impact of that change.

[shinrich]

It seems that the schedule operations in Http2ClientSession have the same issue.

[vmamidi]

Although the fix makes sense, I am concerned that a thread might be blocked if we have any other cases doing the same thing.

[vmamidi]

This looks good, but we should also look at other places for similar issues to avoid any deadlocks.

[shinrich]

I think doing using the TSHttpSsnCallback class in TSHttpSsnReenable() would be the safe thing to do if the lock cannot be immediately acquired. A new continuation is created using the same mutex as the original target continuation. The only purpose of the Callback continuation is to call the handler of the original target continuation with the original arguments and then clean itself up. With a reuse list of jemalloc and/or a proxy allocator list, the memory manipulation overhead should be minimal.

[maskit]

The code was introduced by 85c0211. All 7.x and 8.x releases should be affected.

Also, _switch_thread_if_not_on_right_thread might not safe neither. I'm not sure if there are components that call Http2Stream::handleEvent directly, but if there were, _switch_thread_if_not_on_right_thread might not work as expected. The function itself is relatively new but the logic came from the same commit above.

Always using handleEvent would work for HttpSM and HttpTunnel, but we cannot do the same for _switch_thread_if_not_on_right_thread.

[maskit]

ProxyTransaction::adjust_thread might cause the same issue. It's basically does the same thing as _switch_thread_if_not_on_right_thread.