bryancall commented on PR #13652:
URL: https://github.com/apache/trafficserver/pull/13652#issuecomment-5593598316
I have made this confusing. Let me reset it, and apologize: I asked you for
something in my review that I then took back, and you had already done the
work.
That is my fault, not yours.
## Where this stands
Your original revision (3f5cfa1315) was green on all 15 checks and is the
right
change. `AuTest 1of4` went red only after dccd8f0658, and only on this PR. I
checked the seven other open PRs and 1of4 passes on all of them, so it is not
infrastructure.
dccd8f0658 does two separable things:
1. Removes the 1750 match-limit override. This is the regression fix, it is
correct, and it should merge.
2. Switches the plugin to the shared Regex context, which also gives it a 1
MiB
just-in-time stack instead of PCRE2's fallback 32 KiB.
I asked for (2). I should not have asked for it here. It makes a URL that
has not
redirected since 2019 start redirecting, and in doing so it retires the crash
regression test from #5762, where Alan wrote "without it, the second request
will
crash ATS". That is a real behavior change and a real test being removed,
and it
deserves its own PR rather than riding along with a regression fix.
## What I would do
Add a new commit. Please do not amend and force push again, so the diff stays
reviewable.
**Revert just these two things back to how 3f5cfa1315 had them:**
- `plugins/regex_remap/regex_remap.cc`: restore the per-instance
`RegexMatchContext match_context` member and passing
`&(ri->match_context)` to
`re->match()`. Keep the removal of `REGEX_MATCH_LIMIT` and its
`set_match_limit()` call, which is the actual fix.
- The 3071 byte `/alpha/bravo/` transaction: put back the non-redirecting
expectation against `gold/regex_remap_crash.gold`. Keep the 19 to 32
change on
the `/match_limit/` subject, which is necessary and correct.
**Keep everything else in dccd8f0658.** All of it is independent of the
context
change and all of it is an improvement:
- Backport qualified to 10.2.x only
- `+=` instead of `=` on the two diags_log assertions
- the ESI cross-reference removal
- the documented 2000 byte minimum on the regression subject
- the wording fixes, and the long_query.conf comments
- #13654 filed for the resource-error fallthrough
## What I will do
I will open the shared-context change as its own PR, with the resource-limit
behavior asserted in `test_Regex.cc` rather than through curl and gold
files, and
with a replacement for the #5762 crash guard. Two reasons it belongs in a
unit
test: it runs in milliseconds without a server, and more importantly the
AuTest
cannot see what it is testing. A recursion-limit error, a work-limit error, a
just-in-time stack error, and no limit at all all render as `HTTP/1.1 200
OK` to
curl. That is why the assertion in that test drifted from -21 to -47 across
two
engine conversions while the transaction now actually emits -46.
None of that is yours to carry. Your change fixes a ten month old regression
and
adds the first test this plugin has ever had for the ordinary case, which is
why
it caught something two conversions did not.
--
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]
