cmcfarlen commented on PR #13406:
URL: https://github.com/apache/trafficserver/pull/13406#issuecomment-5220313792
Both of my earlier points are addressed, and I verified them rather than
taking the replies at face value. From my side this is clear.
**Global lock → per-limiter.** Confirmed: the only lock acquisition this PR
adds is `std::lock_guard<std::mutex> lock(_queue_lock)` inside the new
`remove()`, and `_queue_lock`/`_active_lock` are pre-existing per-limiter
members (`limiter.h:407`, already used by `reserve()`, `free()`, `push()` and
`pop()`). Grepping the diff for any `static`/global mutex turns up nothing. So
`remove()` is exactly as granular as the methods it sits next to, and my
serialization concern does not apply.
**Queue depth.** The inline comment on `remove()` now records the caveat
where someone reading the O(n) erase will actually see it — that a `queue:`
block without a `size:` leaves `_max_queue` at `UINT32_MAX` so `full()` never
trips, leaving `proxy.config.net.connections_throttle` as the only real bound.
With #13511 filed to fix the default and revisit the container, that is the
right split: this PR stays a bug fix and the questionable default gets handled
on its own.
While re-reading I went looking for a residual race and convinced myself it
is closed, which is worth recording because it is the subtle part of the
change. My worry was the check-then-act at the close site:
```cpp
if (!limiter->remove(vc)) {
limiter->free();
}
```
`remove()` works under `_queue_lock` and `free()` under `_active_lock`, so
the decision and the decrement are not atomic with respect to each other. If
the sweep could dequeue a VC and only afterwards reserve its slot, a close
landing in that window would see "not queued", call `free()`, and decrement a
slot nothing had taken — the unmatched decrement that wraps the counter and
makes the next `reserve()` trip `TSReleaseAssert(_active <= _limit)`, exactly
the abort your test drives.
Reversing the sweep to reserve *before* dequeuing is what closes it, and the
`nullptr == vc` give-back handles the case where the queue emptied underneath:
```cpp
while (limiter->size() > 0 && limiter->reserve() == ReserveStatus::RESERVED)
{
auto [vc, contp, start_time] = limiter->pop();
if (nullptr == vc) { // A concurrent close emptied the queue; give the
slot back
limiter->free();
break;
}
```
That makes the three cases add up: still queued, `remove()` returns true and
no slot is released because none was held; already resumed, `remove()` returns
false and `free()` releases precisely the slot the sweep granted; sweep raced
and got nothing, the slot goes straight back. The `max_age` expiry loop is
consistent too, since it pops without reserving and detaches with
`TSUserArgSet(vc, gVCIdx, nullptr)` so the eventual `VCONN_CLOSE` cannot
release a slot it never had. The "Reserving before dequeuing means a resumed VC
owns the slot it was granted" comment is doing real work — worth keeping
verbatim.
The `TLSEventSupport.cc` half reads correctly as well: with a connection
closing while parked in a handshake hook, `curHook` still points into the
handshake hook's list, and since each hook id owns a separate list, advancing
it would walk the handshake list instead of the close list — dropping the close
event or handing it to the wrong plugin. Resetting to `nullptr` unless already
in `HANDSHAKE_HOOKS_DONE` (i.e. unless already iterating the close list) is the
right guard, and picking up `TS_EVENT_VCONN_OUTBOUND_CLOSE` alongside
`TS_EVENT_VCONN_CLOSE` closes the same gap on the outbound side.
I will leave the formal approval to a separate action so my
`CHANGES_REQUESTED` stops blocking. One process note, purely about release
timing rather than the code: this grew from a focused rate_limit accounting fix
to +621/-5 across 11 files with the VCONN_CLOSE delivery work folded in, and
10.2.0 is days from its RC. The two halves are genuinely related — the
parked-hook close is what produces the queued-close accounting path — so I am
not asking for a split on technical grounds. It is worth an explicit RM call on
whether this lands in 10.2.0 or 10.2.1 given the Core/TLS blast radius.
--
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]
