bneradt commented on code in PR #13681: URL: https://github.com/apache/trafficserver/pull/13681#discussion_r4017585538
########## tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh: ########## @@ -20,59 +20,182 @@ # and check that the active-slot counter stays balanced and the server survives. # # 1. holder completes its handshake and holds the single slot (counter = 1); -# 2. one connection enqueues because the slot is full, then closes while parked; -# 3. the sweep reserves a slot and resumes a queued connection; -# 4. the holder is closed and releases its slot; +# 2. one connection enqueues because the slot is full and stays parked at the ClientHello hook; +# 3. the sweep runs while the slot is still held and must leave the queued connection parked; +# 4. the holder is closed and releases its slot; the next sweep reserves that slot and resumes +# the queued connection, which is then closed and releases the slot it now owns; # 5. a probe connection reserves the freed slot. # -# Against the plugin before 508c1bea26 this aborts the server: the sweep resumed a queued -# connection without a reservation, whose close then decremented the counter unmatched until it -# wrapped and reserve() tripped TSReleaseAssert(_active <= _limit). The test asserts the counter -# never wraps and no signal is logged, so it pins that fix as well as this change. +# Against the plugin before 508c1bea26 the sweep resumed the queued connection in step 3 without +# a reservation. Step 4 asserts that reservation and fails there, naming the defect itself. Left +# to run, that same resume aborts the server: the holder's close lands the counter on zero and the +# resumed connection's close, releasing a slot it never held, wraps it below zero, so the probe's +# reserve() trips TSReleaseAssert(_active <= _limit). The test's traffic.out testers reject that +# wrap and that abort however they arise, and stay the backstop for both. # -# args: host port sni +# The connection is resumed and then closed, never closed while parked, because a client cannot +# close a parked connection as far as ATS is concerned: while the ClientHello hook is invoked ATS +# does not read the socket, so a FIN sits in the kernel until the sweep reenables the VC. The +# close-while-queued branch of sni_limiter.cc is therefore out of scope here and cannot be driven +# from a client; do not reintroduce a "closes while parked" step, it only ever resolved to the +# resume-then-close path this script now drives deterministically. +# +# Every step waits for the plugin's own debug line in traffic.out rather than sleeping for a +# guessed interval: on a loaded runner a fixed sleep let the second connection miss the holder +# entirely, so the queue path was silently never exercised (#13679). Every connection reads Review Comment: See...this history is not going to be helpful to the user reading this. A summary of what it does is OK, but most importantly is a description of why it does something if it is non-obvious. Describing how it used to do something wrong earlier is going to be noise that is not helpful here to the user. ########## tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh: ########## @@ -20,59 +20,182 @@ # and check that the active-slot counter stays balanced and the server survives. # # 1. holder completes its handshake and holds the single slot (counter = 1); -# 2. one connection enqueues because the slot is full, then closes while parked; -# 3. the sweep reserves a slot and resumes a queued connection; -# 4. the holder is closed and releases its slot; +# 2. one connection enqueues because the slot is full and stays parked at the ClientHello hook; +# 3. the sweep runs while the slot is still held and must leave the queued connection parked; +# 4. the holder is closed and releases its slot; the next sweep reserves that slot and resumes +# the queued connection, which is then closed and releases the slot it now owns; # 5. a probe connection reserves the freed slot. # -# Against the plugin before 508c1bea26 this aborts the server: the sweep resumed a queued -# connection without a reservation, whose close then decremented the counter unmatched until it -# wrapped and reserve() tripped TSReleaseAssert(_active <= _limit). The test asserts the counter -# never wraps and no signal is logged, so it pins that fix as well as this change. +# Against the plugin before 508c1bea26 the sweep resumed the queued connection in step 3 without +# a reservation. Step 4 asserts that reservation and fails there, naming the defect itself. Left +# to run, that same resume aborts the server: the holder's close lands the counter on zero and the +# resumed connection's close, releasing a slot it never held, wraps it below zero, so the probe's +# reserve() trips TSReleaseAssert(_active <= _limit). The test's traffic.out testers reject that +# wrap and that abort however they arise, and stay the backstop for both. Review Comment: Same comment as above - and it looks like the original here had git commit history in it. ########## tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue.test.py: ########## @@ -1,9 +1,16 @@ ''' -Regression test for a queue-accounting balance bug in the rate_limit SNI limiter: a -queued connection never reserves a slot, but its VCONN_CLOSE unconditionally releases -one, so a queued connection that closes underflows the active-slot counter and the next -reserve() trips a release assertion, aborting the server. ATS must survive the queue -churn. +Regression test for the rate_limit SNI limiter's queue accounting. A queued connection never +reserves a slot, so the sweep must reserve one before resuming it and a close must release +only a slot the connection owns. Before 508c1bea26 the sweep resumed a queued connection +while the limiter was still full: the holder's close then landed the active-slot counter on +zero, the resumed connection's close released a slot it never held and wrapped the counter +below zero, and the next reserve() tripped a release assertion that aborted the server. ATS +must survive the queue churn without the counter wrapping. Review Comment: Let's clean up the comment to not have the github history in it - if people are interested they can find this PR with a blame. -- 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]
