Skip to content

Commit 61c88a8

Browse files
bochencwxbochencwx
authored
Bugfix: Fix some HTTP-related issues (#152)
- Fix the issue where metrics reports as correct when the HTTP client's asynchronous call returns an erroneous HTTP response code - Fix the issue of rpcz not correctly obtaining the size of the HTTP client response packet - Fix the case sensitivity issue in key comparison for HttpHeader.SetIfNotPresent interfac
1 parent 94f686e commit 61c88a8

7 files changed

Lines changed: 179 additions & 12 deletions

File tree

‎trpc/client/http/http_service_proxy.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,14 +68,15 @@ Future<ProtocolPtr> HttpServiceProxy::AsyncInnerUnaryInvoke(const ClientContextP
6868
ProtocolPtr p = rsp.GetValue0();
6969
// When the call is successful, set the response data (for use by the `CLIENT_POST_RPC_INVOKE` filter).
7070
context->SetResponseData(&p);
71-
filter_controller_.RunMessageClientFilters(FilterPoint::CLIENT_POST_RPC_INVOKE, context);
7271
if (!CheckHttpResponse(context, p)) {
7372
std::string error = fmt::format("service name:{},check http reply failed,{}", GetServiceName(),
7473
context->GetStatus().ToString());
7574
TRPC_LOG_ERROR(error);
75+
filter_controller_.RunMessageClientFilters(FilterPoint::CLIENT_POST_RPC_INVOKE, context);
7676
return MakeExceptionFuture<ProtocolPtr>(
7777
CommonException(context->GetStatus().ErrorMessage().c_str(), context->GetStatus().GetFuncRetCode()));
7878
}
79+
filter_controller_.RunMessageClientFilters(FilterPoint::CLIENT_POST_RPC_INVOKE, context);
7980
return MakeReadyFuture<ProtocolPtr>(std::move(p));
8081
}
8182

‎trpc/client/http/http_service_proxy_test.cc‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "trpc/client/make_client_context.h"
2121
#include "trpc/client/service_proxy_option_setter.h"
2222
#include "trpc/codec/codec_manager.h"
23+
#include "trpc/filter/filter_manager.h"
2324
#include "trpc/future/future_utility.h"
2425
#include "trpc/naming/trpc_naming_registry.h"
2526
#include "trpc/proto/testing/helloworld.pb.h"
@@ -28,6 +29,32 @@
2829

2930
namespace trpc::testing {
3031

32+
class TestClientFilter : public trpc::MessageClientFilter {
33+
public:
34+
std::string Name() override { return "test_filter"; }
35+
36+
std::vector<trpc::FilterPoint> GetFilterPoint() override {
37+
std::vector<FilterPoint> points = {trpc::FilterPoint::CLIENT_PRE_RPC_INVOKE,
38+
trpc::FilterPoint::CLIENT_POST_RPC_INVOKE};
39+
return points;
40+
}
41+
42+
void operator()(trpc::FilterStatus& status, trpc::FilterPoint point, const trpc::ClientContextPtr& context) override {
43+
status = FilterStatus::CONTINUE;
44+
// record the status upon entering the CLIENT_POST_RPC_INVOKE point
45+
if (point == FilterPoint::CLIENT_POST_RPC_INVOKE) {
46+
status_ = context->GetStatus();
47+
}
48+
}
49+
50+
void SetStatus(trpc::Status&& status) { status_ = std::move(status); }
51+
52+
Status GetStatus() { return status_; }
53+
54+
private:
55+
trpc::Status status_;
56+
};
57+
3158
class HttpServiceProxyTest : public ::testing::Test {
3259
public:
3360
static void SetUpTestCase() {
@@ -46,6 +73,10 @@ class HttpServiceProxyTest : public ::testing::Test {
4673
codec::Init();
4774
serialization::Init();
4875
naming::Init();
76+
77+
filter_ = std::make_shared<TestClientFilter>();
78+
trpc::FilterManager::GetInstance()->AddMessageClientFilter(filter_);
79+
option_->service_filters.push_back(filter_->Name());
4980
}
5081

5182
static void TearDownTestCase() {
@@ -58,9 +89,11 @@ class HttpServiceProxyTest : public ::testing::Test {
5889

5990
protected:
6091
static std::shared_ptr<ServiceProxyOption> option_;
92+
static std::shared_ptr<TestClientFilter> filter_;
6193
};
6294

6395
std::shared_ptr<ServiceProxyOption> HttpServiceProxyTest::option_ = std::make_shared<ServiceProxyOption>();
96+
std::shared_ptr<TestClientFilter> HttpServiceProxyTest::filter_;
6497

6598
class MockHttpServiceProxy : public trpc::http::HttpServiceProxy {
6699
public:
@@ -3891,4 +3924,107 @@ TEST_F(HttpServiceProxyTest, ConstructPBRequest) {
38913924
}
38923925
}
38933926

3927+
TEST_F(HttpServiceProxyTest, FilterExecWhenSuccess) {
3928+
auto proxy = std::make_shared<MockHttpServiceProxy>();
3929+
proxy->SetMockServiceProxyOption(option_);
3930+
auto ctx = MakeClientContext(proxy);
3931+
ctx->SetStatus(Status());
3932+
ctx->SetAddr("127.0.0.1", 10002);
3933+
3934+
trpc::http::HttpResponse reply;
3935+
reply.SetVersion("1.1");
3936+
reply.SetStatus(trpc::http::HttpResponse::StatusCode::kOk);
3937+
proxy->SetReplyError(false);
3938+
proxy->SetReply(reply);
3939+
3940+
std::string rspStr;
3941+
filter_->SetStatus(Status());
3942+
auto st = proxy->GetString(ctx, "http://127.0.0.1:10002/hello", &rspStr);
3943+
ASSERT_TRUE(st.OK());
3944+
// verify that the status is OK upon entering the RPC post-filter point
3945+
ASSERT_TRUE(filter_->GetStatus().OK());
3946+
3947+
proxy->SetReplyError(false);
3948+
proxy->SetReply(reply);
3949+
ctx = MakeClientContext(proxy);
3950+
ctx->SetStatus(Status());
3951+
ctx->SetAddr("127.0.0.1", 10002);
3952+
filter_->SetStatus(Status());
3953+
auto async_get_string_fut =
3954+
proxy->AsyncGetString(ctx, "http://127.0.0.1:10002/hello").Then([](Future<std::string>&& future) {
3955+
EXPECT_TRUE(future.IsReady());
3956+
// verify that the status is OK upon entering the RPC post-filter point
3957+
EXPECT_TRUE(filter_->GetStatus().OK());
3958+
3959+
return MakeReadyFuture<>();
3960+
});
3961+
future::BlockingGet(std::move(async_get_string_fut));
3962+
}
3963+
3964+
TEST_F(HttpServiceProxyTest, FilterExecWithFrameworkErr) {
3965+
auto proxy = std::make_shared<MockHttpServiceProxy>();
3966+
proxy->SetMockServiceProxyOption(option_);
3967+
auto ctx = MakeClientContext(proxy);
3968+
ctx->SetStatus(Status());
3969+
ctx->SetAddr("127.0.0.1", 10002);
3970+
3971+
proxy->SetReplyError(true);
3972+
std::string rspStr;
3973+
filter_->SetStatus(Status());
3974+
auto st = proxy->GetString(ctx, "http://127.0.0.1:10002/hello", &rspStr);
3975+
ASSERT_FALSE(st.OK());
3976+
// verify that the status is already failed upon entering the RPC post-filter point
3977+
ASSERT_FALSE(filter_->GetStatus().OK());
3978+
3979+
proxy->SetReplyError(true);
3980+
ctx = MakeClientContext(proxy);
3981+
ctx->SetStatus(Status());
3982+
ctx->SetAddr("127.0.0.1", 10002);
3983+
filter_->SetStatus(Status());
3984+
auto async_get_string_fut =
3985+
proxy->AsyncGetString(ctx, "http://127.0.0.1:10002/hello").Then([](Future<std::string>&& future) {
3986+
EXPECT_TRUE(future.IsFailed());
3987+
// verify that the status is already failed upon entering the RPC post-filter point
3988+
EXPECT_FALSE(filter_->GetStatus().OK());
3989+
return MakeReadyFuture<>();
3990+
});
3991+
future::BlockingGet(std::move(async_get_string_fut));
3992+
}
3993+
3994+
TEST_F(HttpServiceProxyTest, FilterExecWithHttpErr) {
3995+
auto proxy = std::make_shared<MockHttpServiceProxy>();
3996+
proxy->SetMockServiceProxyOption(option_);
3997+
auto ctx = MakeClientContext(proxy);
3998+
ctx->SetStatus(Status());
3999+
ctx->SetAddr("127.0.0.1", 10002);
4000+
4001+
trpc::http::HttpResponse reply;
4002+
reply.SetVersion("1.1");
4003+
reply.SetStatus(trpc::http::HttpResponse::StatusCode::kForbidden);
4004+
proxy->SetReplyError(false);
4005+
proxy->SetReply(reply);
4006+
4007+
std::string rspStr;
4008+
filter_->SetStatus(Status());
4009+
auto st = proxy->GetString(ctx, "http://127.0.0.1:10002/hello", &rspStr);
4010+
ASSERT_FALSE(st.OK());
4011+
// verify that the status is already failed upon entering the RPC post-filter point
4012+
ASSERT_FALSE(filter_->GetStatus().OK());
4013+
4014+
proxy->SetReplyError(false);
4015+
proxy->SetReply(reply);
4016+
ctx = MakeClientContext(proxy);
4017+
ctx->SetStatus(Status());
4018+
ctx->SetAddr("127.0.0.1", 10002);
4019+
filter_->SetStatus(Status());
4020+
auto async_get_string_fut =
4021+
proxy->AsyncGetString(ctx, "http://127.0.0.1:10002/hello").Then([](Future<std::string>&& future) {
4022+
EXPECT_TRUE(future.IsFailed());
4023+
// verify that the status is already failed upon entering the RPC post-filter point
4024+
EXPECT_FALSE(filter_->GetStatus().OK());
4025+
return MakeReadyFuture<>();
4026+
});
4027+
future::BlockingGet(std::move(async_get_string_fut));
4028+
}
4029+
38944030
} // namespace trpc::testing

‎trpc/codec/http/http_protocol.cc‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@ bool HttpRequestProtocol::WaitForFullRequest() {
5757
return false;
5858
}
5959

60+
uint32_t HttpRequestProtocol::GetMessageSize() const { return request->ContentLength(); }
61+
62+
uint32_t HttpResponseProtocol::GetMessageSize() const { return response.ContentLength(); }
63+
6064
namespace internal {
6165
const std::string& GetHeaderString(const http::HeaderPairs& header, const std::string& name) {
6266
return header.Get(name);

‎trpc/codec/http/http_protocol.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,9 @@ class HttpRequestProtocol : public Protocol {
7272
void SetFromHttpServiceProxy(bool from_http_service_proxy) { from_http_service_proxy_ = from_http_service_proxy; }
7373
bool IsFromHttpServiceProxy() { return from_http_service_proxy_; }
7474

75+
/// @brief Get size of message
76+
uint32_t GetMessageSize() const override;
77+
7578
public:
7679
uint32_t request_id{0};
7780
http::RequestPtr request{nullptr};
@@ -101,6 +104,9 @@ class HttpResponseProtocol : public Protocol {
101104
return std::move(*response.GetMutableNonContiguousBufferContent());
102105
}
103106

107+
/// @brief Get size of message
108+
uint32_t GetMessageSize() const override;
109+
104110
public:
105111
http::Response response;
106112
};

‎trpc/codec/http/http_protocol_test.cc‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,9 @@ TEST_F(HttpProtoFixture, HttpRequestProtocolTest) {
3535
HttpRequestProtocol req = HttpRequestProtocol(std::make_shared<http::Request>());
3636
req.request->SetContent(test);
3737
NoncontiguousBuffer buff;
38-
EXPECT_FALSE(req.ZeroCopyDecode(buff));
39-
EXPECT_FALSE(req.ZeroCopyEncode(buff));
38+
ASSERT_FALSE(req.ZeroCopyDecode(buff));
39+
ASSERT_FALSE(req.ZeroCopyEncode(buff));
40+
ASSERT_NE(0, req.GetMessageSize());
4041
}
4142

4243
TEST_F(HttpProtoFixture, HttpResponseProtocolTest) {
@@ -47,8 +48,9 @@ TEST_F(HttpProtoFixture, HttpResponseProtocolTest) {
4748
reply.SetStatus(trpc::http::HttpResponse::StatusCode::kOk);
4849
reply.SetContent("{\"age\":\"18\",\"height\":180}");
4950
NoncontiguousBuffer buff;
50-
EXPECT_FALSE(rsp.ZeroCopyDecode(buff));
51-
EXPECT_FALSE(rsp.ZeroCopyEncode(buff));
51+
ASSERT_FALSE(rsp.ZeroCopyDecode(buff));
52+
ASSERT_FALSE(rsp.ZeroCopyEncode(buff));
53+
ASSERT_NE(0, rsp.GetMessageSize());
5254
}
5355

5456
TEST(HttpRequestProtocolTest, GetOkNonContiguousProtocolBody) {
@@ -59,13 +61,13 @@ TEST(HttpRequestProtocolTest, GetOkNonContiguousProtocolBody) {
5961
request_protocol.SetNonContiguousProtocolBody(builder.DestructiveGet());
6062

6163
auto body_buffer = request_protocol.GetNonContiguousProtocolBody();
62-
EXPECT_EQ(greetings.size(), body_buffer.ByteSize());
64+
ASSERT_EQ(greetings.size(), body_buffer.ByteSize());
6365
}
6466

6567
TEST(HttpRequestProtocolTest, GetEmptyNonContiguousProtocolBody) {
6668
HttpRequestProtocol request_protocol{std::make_shared<http::HttpRequest>()};
6769
auto body_buffer = request_protocol.GetNonContiguousProtocolBody();
68-
EXPECT_EQ(0, body_buffer.size());
70+
ASSERT_EQ(0, body_buffer.size());
6971
}
7072

7173
TEST(HttpResponseProtocolTest, GetOkNonContiguousProtocolBody) {
@@ -74,8 +76,8 @@ TEST(HttpResponseProtocolTest, GetOkNonContiguousProtocolBody) {
7476

7577
response_protocol.SetNonContiguousProtocolBody(CreateBufferSlow(greetings));
7678

77-
EXPECT_EQ(greetings.size(), response_protocol.GetNonContiguousProtocolBody().ByteSize());
78-
EXPECT_TRUE(response_protocol.response.GetContent().empty());
79+
ASSERT_EQ(greetings.size(), response_protocol.GetNonContiguousProtocolBody().ByteSize());
80+
ASSERT_TRUE(response_protocol.response.GetContent().empty());
7981
}
8082

8183
TEST(EncodeTypeToMimeTest, EncodeTypeToMime) {
@@ -92,7 +94,7 @@ TEST(EncodeTypeToMimeTest, EncodeTypeToMime) {
9294
};
9395

9496
for (const auto& t : testings) {
95-
EXPECT_EQ(t.expect, EncodeTypeToMime(t.encode_type));
97+
ASSERT_EQ(t.expect, EncodeTypeToMime(t.encode_type));
9698
}
9799
}
98100

@@ -112,7 +114,7 @@ TEST(MimeToEncodeTypeTest, MimeToEncodeType) {
112114
};
113115

114116
for (const auto& t : testings) {
115-
EXPECT_EQ(t.expect, MimeToEncodeType(t.mime));
117+
ASSERT_EQ(t.expect, MimeToEncodeType(t.mime));
116118
}
117119
}
118120

‎trpc/util/http/field_map.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ class FieldMap {
7979
/// @brief Does same thing as Set method, but only works when key does not exist in the map.
8080
template <typename K, typename V>
8181
void SetIfNotPresent(K&& key, V&& value) {
82-
if (auto it = pairs_.lower_bound(key); it == pairs_.end() || it->first != key) {
82+
if (auto it = pairs_.lower_bound(key); it == pairs_.end() || !CaseInsensitiveEqualTo()(it->first, key)) {
8383
pairs_.emplace_hint(it, std::forward<K>(key), std::forward<V>(value));
8484
}
8585
}

‎trpc/util/http/field_map_test.cc‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,4 +207,22 @@ TEST_F(FieldMapTest, FlatPairsCount) {
207207
ASSERT_EQ(17, header_.FlatPairsCount());
208208
}
209209

210+
TEST_F(FieldMapTest, SetIfNotPresentOk) {
211+
EXPECT_EQ(0, header_.Values("User-Defined-Key99").size());
212+
header_.SetIfNotPresent("User-Defined-Key99", "user-defined-value99");
213+
EXPECT_EQ(1, header_.Values("User-Defined-Key99").size());
214+
EXPECT_EQ("user-defined-value99", header_.Get("User-Defined-Key99"));
215+
216+
EXPECT_EQ(1, header_.Values("User-Defined-Key01").size());
217+
header_.SetIfNotPresent("User-Defined-Key01", "user-defined-value01-new");
218+
EXPECT_EQ(1, header_.Values("User-Defined-Key01").size());
219+
EXPECT_EQ("user-defined-value01", header_.Get("User-Defined-Key01"));
220+
221+
// case insensitivity test
222+
EXPECT_EQ(1, header_.Values("user-defined-key01").size());
223+
header_.SetIfNotPresent("user-defined-key01", "user-defined-value01-new");
224+
EXPECT_EQ(1, header_.Values("user-defined-key01").size());
225+
EXPECT_EQ("user-defined-value01", header_.Get("user-defined-key01"));
226+
}
227+
210228
} // namespace trpc::http::testing

0 commit comments

Comments
 (0)