Cleanup: remove ts::Buffer from ControlBase.

[SolidWallOfCode]

Remove and replace ts::Buffer.

[bneradt]

It seems like somehow your PR is impacting the behavior of the immediate shutdown oriented tests. The ones that misconfigure ATS then expect certain logs and response codes:

https://ci.trafficserver.apache.org/job/Github_Builds/job/autest/6935/console

Test : Checking that ReturnCode == 33 - Failed
        Reason: Returned Value -2 != 33
     Time-Out : Process finishes within expected time - Passed
        Reason: Returned value: 1.0000324249267578 < 600.0
     file /tmp/sandbox/emergency/ts/log/diags.log : should contain "testing emergency shutdown" - Failed
        Reason: Cannot read /tmp/sandbox/emergency/ts/log/diags.log: [Errno 2] No such file or directory: '/tmp/sandbox/emergency/ts/log/diags.log'
     file /tmp/sandbox/emergency/ts/log/traffic.out : should NOT contain "failed to shutdown" - Failed
        Reason: Cannot read /tmp/sandbox/emergency/ts/log/traffic.out: [Errno 2] No such file or directory: '/tmp/sandbox/emergency/ts/log/traffic.out'

Note the difference in return code (-2 instead of 33) and the diags.log and traffic.out files are not there. These tests are generally pretty reliable in master - your other recent PRs haven't failed these tests.

Unfortunately, the archived sandbox doesn't have much information, since none of the logs were emitted:

https://ci.trafficserver.apache.org/job/Github_Builds/job/autest/6935/artifact/output/9664/sandbox/emergency/

[bneradt]

We need to investigate why the shutdown autests are having issues in CI with this PR.

[SolidWallOfCode]

@bneradt - actually, they've all failed and are now successful only because of retries.

[bneradt]

@bneradt - actually, they've all failed and are now successful only because of retries.

Exactly, some passed because of a retry. This is what I notice:

• Your first run for this PR had some shutdown test failures.
• Your second had a shutdown failure which I analyzed and commented upon above.
• You have three or so other PRs you filed today which have had clean runs without any retries.

My observation is that it seems like this patch is causing issues with some of the shutdown tests (fatal.test.py, emergency.test.py, etc.). I'm not sure why that's the case, but I don't want us to just re-run autest a bunch of times, get this merged in once all the tests pass by happenstance, then destabilize those tests for the rest of CI.

[bneradt]

@bneradt - actually, they've all failed and are now successful only because of retries.

Maybe I misunderstood you. When you say they all failed are you referring to CI for the other PR's?

[SolidWallOfCode]

Yes, for every one of these PRs I have had to run CI multiple times. For instance #9661 took three tries. Note on this run, it was a failure on Rocky - all of the AuTests passed.

[bneradt]

Yes, for every one of these PRs I have had to run CI multiple times. For instance #9661 took three tries. Note on this run, it was a failure on Rocky - all of the AuTests passed.

Ah, OK. Then I take back my comment. This PR isn't the problem then. Clearly we'll need to figure out what has caused CI to not be stable anymore. It was running pretty consistently for the last couple months.

[bneradt]

[approve ci rocky]

[bneradt]

The PR looks fine as is. But when reviewing the code, doesn't this need to call free on these sowc::memSpan elements?

https://github.com/apache/trafficserver/pull/9664/files#diff-222342fe7ac7184aa61268fff5299a17c55b3984daeba4ee796ba8ab9ca482cfR445

Clearing the vector isn't going to free them. Or are they free'd elsewhere?

[SolidWallOfCode]

That looks like a leak. I suspect we don't see it in the field because

• Those structures are process static, so it's only "leaked" on process shutdown.
• I'm not sure anything actually uses the multiple text matcher.

The correct solution is to leverage YAML to get rid of the entire ControlMatcher library, but short of that it would be better to put the text in a std::string member and have views into that, rather than using ats_strdup.

[bneradt]

The change looks fine. Again, you didn't introduce the leak here.

We can address the leak in a separate PR if you prefer. Using a std::string as you suggest makes sense. Or we can loop over the vector and call free on them.

[SolidWallOfCode]

No, looping over the vector is the wrong approach. It validates doing separate allocations for each string, instead of one for all of them. Don't you remember @zwoop was whining about exactly that at the last PR review?