Skip to content

Commit 19b83cc

Browse files
Victor Linfacebook-github-bot
Victor Lin
authored and
facebook-github-bot
committed
Convert string constants in THeader to string_view
Summary: There are some static strings used as header names in THeader that should be changed to use the std::string_view class to avoid SIOF. Updates all related callsites of these headers to take in string_view instead of string. Reviewed By: iahs Differential Revision: D39100059 fbshipit-source-id: 41a65b0ede98d80a246a470d11aea967b970ca6c
1 parent 53f62a0 commit 19b83cc

File tree

5 files changed

+29
-31
lines changed

5 files changed

+29
-31
lines changed

‎third-party/thrift/src/thrift/lib/cpp/transport/THeader.cpp

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -64,15 +64,7 @@ using namespace folly;
6464
using namespace folly::io;
6565
using apache::thrift::protocol::TBinaryProtocol;
6666

67-
const string THeader::IDENTITY_HEADER = "identity";
68-
const string THeader::ID_VERSION_HEADER = "id_version";
69-
const string THeader::ID_VERSION = "1";
70-
const string THeader::PRIORITY_HEADER = "thrift_priority";
7167
const string& THeader::CLIENT_TIMEOUT_HEADER = *(new string("client_timeout"));
72-
const string THeader::QUEUE_TIMEOUT_HEADER = "queue_timeout";
73-
const string THeader::QUERY_LOAD_HEADER = "load";
74-
const string THeader::kClientId = "client_id";
75-
const string THeader::kServiceTraceMeta = "service_trace_meta";
7668

7769
std::string getReadableChars(Cursor c, size_t limit) {
7870
size_t size = 0;
@@ -696,11 +688,11 @@ static void flushInfoHeaders(
696688
}
697689
}
698690

699-
void THeader::setHeader(const std::string& key, const std::string& value) {
691+
void THeader::setHeader(std::string_view key, const std::string& value) {
700692
ensureWriteHeaders()[key] = value;
701693
}
702694

703-
void THeader::setHeader(const std::string& key, std::string&& value) {
695+
void THeader::setHeader(std::string_view key, std::string&& value) {
704696
ensureWriteHeaders()[key] = std::move(value);
705697
}
706698

@@ -790,7 +782,7 @@ const THeader::StringToStringMap& THeader::getWriteHeaders() const {
790782
return writeHeaders_ ? *writeHeaders_ : kEmptyMap();
791783
}
792784

793-
void THeader::setReadHeader(const std::string& key, std::string&& value) {
785+
void THeader::setReadHeader(std::string_view key, std::string&& value) {
794786
ensureReadHeaders()[key] = std::move(value);
795787
}
796788

@@ -1006,7 +998,7 @@ apache::thrift::concurrency::PRIORITY THeader::getCallPriority() const {
1006998
}
1007999

10081000
std::chrono::milliseconds THeader::getTimeoutFromHeader(
1009-
const std::string& header) const {
1001+
std::string_view header) const {
10101002
const auto& map = getHeaders();
10111003
auto iter = map.find(header);
10121004
if (iter != map.end()) {

‎third-party/thrift/src/thrift/lib/cpp/transport/THeader.h

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -221,8 +221,8 @@ class THeader final {
221221
std::optional<ClientMetadata> extractClientMetadata();
222222

223223
// these work with write headers
224-
void setHeader(const std::string& key, const std::string& value);
225-
void setHeader(const std::string& key, std::string&& value);
224+
void setHeader(std::string_view key, const std::string& value);
225+
void setHeader(std::string_view key, std::string&& value);
226226
void setHeader(
227227
const char* key, size_t keyLength, const char* value, size_t valueLength);
228228
void setHeaders(StringToStringMap&&);
@@ -235,7 +235,7 @@ class THeader final {
235235

236236
// these work with read headers
237237
void setReadHeaders(StringToStringMap&&);
238-
void setReadHeader(const std::string& key, std::string&& value);
238+
void setReadHeader(std::string_view key, std::string&& value);
239239
void eraseReadHeader(const std::string& key);
240240
const StringToStringMap& getHeaders() const;
241241
StringToStringMap releaseHeaders();
@@ -328,8 +328,7 @@ class THeader final {
328328

329329
apache::thrift::concurrency::PRIORITY getCallPriority() const;
330330

331-
std::chrono::milliseconds getTimeoutFromHeader(
332-
const std::string& header) const;
331+
std::chrono::milliseconds getTimeoutFromHeader(std::string_view header) const;
333332

334333
std::chrono::milliseconds getClientTimeout() const;
335334

@@ -379,12 +378,13 @@ class THeader final {
379378
static const uint32_t BIG_FRAME_MAGIC = 0x42494746; // BIGF
380379

381380
static const uint32_t MAX_FRAME_SIZE = 0x3FFFFFFF;
382-
static const std::string PRIORITY_HEADER;
381+
static constexpr std::string_view PRIORITY_HEADER = "thrift_priority";
382+
// TODO: change to string_view
383383
static const std::string& CLIENT_TIMEOUT_HEADER;
384-
static const std::string QUEUE_TIMEOUT_HEADER;
385-
static const std::string QUERY_LOAD_HEADER;
386-
static const std::string kClientId;
387-
static const std::string kServiceTraceMeta;
384+
static constexpr std::string_view QUEUE_TIMEOUT_HEADER = "queue_timeout";
385+
static constexpr std::string_view QUERY_LOAD_HEADER = "load";
386+
static constexpr std::string_view kClientId = "client_id";
387+
static constexpr std::string_view kServiceTraceMeta = "service_trace_meta";
388388
static constexpr std::string_view CLIENT_METADATA_HEADER = "client_metadata";
389389

390390
private:
@@ -461,9 +461,9 @@ class THeader final {
461461
folly::Optional<std::string> clientId_;
462462
folly::Optional<std::string> serviceTraceMeta_;
463463

464-
static const std::string IDENTITY_HEADER;
465-
static const std::string ID_VERSION_HEADER;
466-
static const std::string ID_VERSION;
464+
static constexpr std::string_view IDENTITY_HEADER = "identity";
465+
static constexpr std::string_view ID_VERSION_HEADER = "id_version";
466+
static constexpr std::string_view ID_VERSION = "1";
467467

468468
bool allowBigFrames_;
469469
folly::Optional<CompressionConfig> compressionConfig_;

‎third-party/thrift/src/thrift/lib/cpp2/async/RpcOptions.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ class RpcOptions {
127127
RpcOptions& setShardId(std::string shardId);
128128
const std::string& getShardId() const;
129129

130+
// TODO: transition this to string_view instead of const string&
130131
void setWriteHeader(const std::string& key, const std::string& value);
131132
const transport::THeader::StringToStringMap& getWriteHeaders() const;
132133
transport::THeader::StringToStringMap releaseWriteHeaders();

‎third-party/thrift/src/thrift/lib/cpp2/test/server/ThriftServerTest.cpp

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -472,7 +472,8 @@ void doLoadHeaderTest(bool isRocket) {
472472
// Empty load header
473473
RpcOptions options;
474474
const std::string kLoadMetric;
475-
options.setWriteHeader(THeader::QUERY_LOAD_HEADER, kLoadMetric);
475+
options.setWriteHeader(
476+
(std::string)THeader::QUERY_LOAD_HEADER, kLoadMetric);
476477
auto [_, header] = client->header_semifuture_voidResponse(options).get();
477478
checkLoadHeader(*header, kLoadMetric);
478479
}
@@ -481,7 +482,8 @@ void doLoadHeaderTest(bool isRocket) {
481482
// Custom load header
482483
RpcOptions options;
483484
const std::string kLoadMetric{"custom_load_metric_789"};
484-
options.setWriteHeader(THeader::QUERY_LOAD_HEADER, kLoadMetric);
485+
options.setWriteHeader(
486+
(std::string)THeader::QUERY_LOAD_HEADER, kLoadMetric);
485487
auto [_, header] = client->header_semifuture_voidResponse(options).get();
486488
checkLoadHeader(*header, kLoadMetric);
487489
}
@@ -490,7 +492,8 @@ void doLoadHeaderTest(bool isRocket) {
490492
// Force server overload. Load should still be returned on server overload.
491493
RpcOptions options;
492494
const std::string kLoadMetric;
493-
options.setWriteHeader(THeader::QUERY_LOAD_HEADER, kLoadMetric);
495+
options.setWriteHeader(
496+
(std::string)THeader::QUERY_LOAD_HEADER, kLoadMetric);
494497

495498
folly::fibers::Baton baton;
496499
THeader header;
@@ -527,7 +530,8 @@ void doLoadHeaderTest(bool isRocket) {
527530
});
528531
RpcOptions options;
529532
const std::string kLoadMetric;
530-
options.setWriteHeader(THeader::QUERY_LOAD_HEADER, kLoadMetric);
533+
options.setWriteHeader(
534+
(std::string)THeader::QUERY_LOAD_HEADER, kLoadMetric);
531535
options.setQueueTimeout(std::chrono::milliseconds(10));
532536

533537
folly::fibers::Baton baton;
@@ -564,7 +568,8 @@ void doLoadHeaderTest(bool isRocket) {
564568

565569
RpcOptions options;
566570
const std::string kLoadMetric;
567-
options.setWriteHeader(THeader::QUERY_LOAD_HEADER, kLoadMetric);
571+
options.setWriteHeader(
572+
(std::string)THeader::QUERY_LOAD_HEADER, kLoadMetric);
568573
options.setTimeout(std::chrono::seconds(1));
569574

570575
auto prevTaskExpireTime = runner.getThriftServer().getTaskExpireTime();

‎third-party/thrift/src/thrift/lib/cpp2/transport/core/testutil/TransportCompatibilityTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ void TransportCompatibilityTest::TestRequestResponse_Header_Load() {
571571
connectToServer([](std::unique_ptr<TestServiceAsyncClient> client) {
572572
RpcOptions rpcOptions;
573573
rpcOptions.setWriteHeader("header_from_client", "2");
574-
rpcOptions.setWriteHeader(THeader::QUERY_LOAD_HEADER, {});
574+
rpcOptions.setWriteHeader((std::string)THeader::QUERY_LOAD_HEADER, {});
575575
auto resultAndHeaders = client->header_future_headers(rpcOptions).get();
576576
const auto readHeaders =
577577
std::move(resultAndHeaders.second)->releaseHeaders();

0 commit comments

Comments
 (0)