Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

use AppendHeader for http2 #2387

Merged
merged 2 commits into from
Oct 7, 2023
Merged

use AppendHeader for http2 #2387

merged 2 commits into from
Oct 7, 2023

Conversation

lhsoft
Copy link
Contributor

@lhsoft lhsoft commented Sep 19, 2023

What problem does this PR solve?

Issue Number:

Problem Summary:

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects(性能影响):

  • Breaking backward compatibility(向后兼容性):


Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

@@ -1544,7 +1544,7 @@ TEST_F(HttpTest, http2_header_after_data) {
ASSERT_EQ(res_header.content_type(), "application/proto");
// Check overlapped header is overwritten by the latter.
const std::string* user_defined1 = res_header.GetHeader("user-defined1");
ASSERT_EQ(*user_defined1, "overwrite-a");
ASSERT_EQ(*user_defined1, "a,overwrite-a");
Copy link
Contributor

Choose a reason for hiding this comment

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

The original test case was written as "overwrite-a", was it expected to be overwritten? Changing the behavior here may not conform to the previous design.

Copy link
Contributor Author

@lhsoft lhsoft Sep 20, 2023

Choose a reason for hiding this comment

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

The test case is http2_header_after_data.
For http2, according to the rfc , it shouldn't be expected to be overwritten.
The grpc use map[string][]string for header:

md := metadata.Pairs(
    "key1", "val1",
    "key1", "val1-2", // "key1" will have map value []string{"val1", "val1-2"}
    "key2", "val2",
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why it was designed to overwrite before? @zyearn

Copy link
Member

Choose a reason for hiding this comment

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

RFC doesn't mention overwritten. It says: Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)].

In brpc implementation, if the incoming header is in the format of "key:value1,value2,value3", brpc won't overwrite cases like this. But combining headers seems reasonable otherwise the previous header would be lost.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@Huixxi Huixxi merged commit 3e87b91 into apache:master Oct 7, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants