Skip to content

compress; Fixes a regression in protocol parser#13107

Merged
zwoop merged 2 commits intoapache:masterfrom
zwoop:CompressFix
Apr 21, 2026
Merged

compress; Fixes a regression in protocol parser#13107
zwoop merged 2 commits intoapache:masterfrom
zwoop:CompressFix

Conversation

@zwoop
Copy link
Copy Markdown
Contributor

@zwoop zwoop commented Apr 20, 2026

This is a regression I think, in 10.2, where we get warnings like

...(Parse)> (compress) WARNING: failed to interpret "br,gzip" at line 2

@zwoop zwoop added this to the 11.0.0 milestone Apr 20, 2026
@zwoop zwoop requested a review from Copilot April 20, 2026 22:44
@zwoop zwoop self-assigned this Apr 20, 2026
@zwoop zwoop added the compress compress plugin label Apr 20, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a regression in the compress plugin configuration parser where list-style directives could leave the remainder of the line to be re-tokenized by the outer parser loop, producing warnings like failed to interpret "br,gzip".

Changes:

  • After parsing supported-algorithms ..., clear the remaining line_view so list tokens aren’t reinterpreted as top-level directives.
  • After parsing compressible-status-code ..., clear the remaining line_view for the same reason.

Comment thread plugins/compress/configuration.cc
bneradt
bneradt previously approved these changes Apr 20, 2026
Copy link
Copy Markdown
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct. I wish these didn't have such similar names, with some consuming and some not:

  void               add_allow(swoc::TextView allow);
  void               add_compressible_content_type(swoc::TextView content_type);
  void               add_compressible_status_codes(swoc::TextView &status_codes);
// ...
  void               add_compression_algorithms(swoc::TextView &algorithms);
//...
  void               set_range_request(swoc::TextView token);

But I suppose the pattern is that those which take multiple tokens take by reference and consume, while those that take a single token don't consume.

Anyway, it should at least fix the bug.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

plugins/compress/configuration.cc:202

  • add_compression_algorithms() now consumes the rest of the line via extractFirstToken(line, isCommaOrSpace), but it will treat trailing inline comments as another token (e.g. supported-algorithms gzip, br # note) and fall into the "Unknown compression type" error path. Consider stopping parsing when the extracted token starts with # (consistent with the main parse loop’s comment handling).
HostConfiguration::add_compression_algorithms(swoc::TextView &line)
{
  compression_algorithms_ = ALGORITHM_DEFAULT; // remove the default gzip.
  for (;;) {
    auto token = extractFirstToken(line, isCommaOrSpace);
    if (token.empty()) {

plugins/compress/configuration.cc:236

  • add_compressible_status_codes() will currently attempt to parse trailing inline comments as a status code token (e.g. compressible-status-code 200, 206 # note), which triggers the "Invalid status code" error. Consider breaking out of the loop when the extracted token starts with # to match the parser’s comment semantics.
HostConfiguration::add_compressible_status_codes(swoc::TextView &line)
{
  compressible_status_codes_.clear();

  for (;;) {
    auto token = extractFirstToken(line, isCommaOrSpace);

Comment thread plugins/compress/configuration.cc
Comment thread plugins/compress/configuration.h
zwoop added 2 commits April 20, 2026 22:13
This is a regression I think, in 10.2, where we get warnings like

...(Parse)> (compress) WARNING: failed to interpret "br,gzip" at line 2

Also address copilots concerns
@zwoop
Copy link
Copy Markdown
Contributor Author

zwoop commented Apr 21, 2026

I tested the compress autest on my mac with current master (without these changes), and it fails the tests when zstd is available and built. I think (definitely not sure) that something in the combinations of the builds, and the real fixes here to the parser, ended up triggering failing these autests.

So, although this code change doesn't change any behavior, other than fixing the parsing failures, it also surfaced errors in the autest gold files as well.

@zwoop zwoop merged commit 450d3f3 into apache:master Apr 21, 2026
15 checks passed
@zwoop zwoop deleted the CompressFix branch April 21, 2026 15:59
@github-project-automation github-project-automation Bot moved this to For v10.2.0 in ATS v10.2.x Apr 21, 2026
@cmcfarlen cmcfarlen moved this from For v10.2.0 to Picked v10.2.0 in ATS v10.2.x Apr 21, 2026
@cmcfarlen cmcfarlen modified the milestones: 11.0.0, 10.2.0 Apr 21, 2026
@cmcfarlen
Copy link
Copy Markdown
Contributor

Cherry-picked to 10.2.x

cmcfarlen pushed a commit that referenced this pull request Apr 21, 2026
* compress; Fixes a regression in protocol parser

This is a regression I think, in 10.2, where we get warnings like

...(Parse)> (compress) WARNING: failed to interpret "br,gzip" at line 2

Also address copilots concerns

* Fix failing autests, after our fixes. This was broken on master before too.

(cherry picked from commit 450d3f3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compress compress plugin

Projects

Status: Picked v10.2.0

Development

Successfully merging this pull request may close these issues.

4 participants