Simplify LoadBalancerServerId by making the constructors public
and adding an IsValid() method. This allows the constructs to create and
"invalid" server id if the input is invalid, which eliminates the
need for the use of std::optional, and the Create() factory methods.
This also simplifies the LoadBalancerDecoder and makes it more efficient
by avoiding std::optional and increase the impact of copy-elision.
PiperOrigin-RevId: 596671653
diff --git a/quiche/quic/load_balancer/load_balancer_decoder.cc b/quiche/quic/load_balancer/load_balancer_decoder.cc
index b1748d6..3cdfd2b 100644
--- a/quiche/quic/load_balancer/load_balancer_decoder.cc
+++ b/quiche/quic/load_balancer/load_balancer_decoder.cc
@@ -36,31 +36,31 @@
// This is the core logic to extract a server ID given a valid config and
// connection ID of sufficient length.
-std::optional<LoadBalancerServerId> LoadBalancerDecoder::GetServerId(
+LoadBalancerServerId LoadBalancerDecoder::GetServerId(
const QuicConnectionId& connection_id) const {
std::optional<uint8_t> config_id = GetConfigId(connection_id);
if (!config_id.has_value()) {
- return std::optional<LoadBalancerServerId>();
+ return LoadBalancerServerId();
}
std::optional<LoadBalancerConfig> config = config_[*config_id];
if (!config.has_value()) {
- return std::optional<LoadBalancerServerId>();
+ return LoadBalancerServerId();
}
if (connection_id.length() < config->total_len()) {
// Connection ID wasn't long enough
- return std::optional<LoadBalancerServerId>();
+ return LoadBalancerServerId();
}
// The first byte is complete. Finish the rest.
const uint8_t* data =
reinterpret_cast<const uint8_t*>(connection_id.data()) + 1;
if (!config->IsEncrypted()) { // It's a Plaintext CID.
- return LoadBalancerServerId::Create(
+ return LoadBalancerServerId(
absl::Span<const uint8_t>(data, config->server_id_len()));
}
uint8_t result[kQuicMaxConnectionIdWithLengthPrefixLength];
if (config->plaintext_len() == kLoadBalancerKeyLen) { // single pass
if (!config->BlockDecrypt(data, result)) {
- return std::optional<LoadBalancerServerId>();
+ return LoadBalancerServerId();
}
} else {
// Do 3 or 4 passes. Only 3 are necessary if the server_id is short enough
@@ -70,11 +70,11 @@
uint8_t end = (config->server_id_len() > config->nonce_len()) ? 1 : 2;
for (uint8_t i = kNumLoadBalancerCryptoPasses; i >= end; i--) {
if (!config->EncryptionPass(absl::Span<uint8_t>(result), i)) {
- return std::optional<LoadBalancerServerId>();
+ return LoadBalancerServerId();
}
}
}
- return LoadBalancerServerId::Create(
+ return LoadBalancerServerId(
absl::Span<const uint8_t>(result, config->server_id_len()));
}
diff --git a/quiche/quic/load_balancer/load_balancer_decoder.h b/quiche/quic/load_balancer/load_balancer_decoder.h
index 11852a3..21eb8b7 100644
--- a/quiche/quic/load_balancer/load_balancer_decoder.h
+++ b/quiche/quic/load_balancer/load_balancer_decoder.h
@@ -40,8 +40,7 @@
// codepoint, |connection_id| is too short, or there's a decrypt error,
// returns empty. Will accept |connection_id| that is longer than necessary
// without error.
- std::optional<LoadBalancerServerId> GetServerId(
- const QuicConnectionId& connection_id) const;
+ LoadBalancerServerId GetServerId(const QuicConnectionId& connection_id) const;
// Returns the config ID stored in the first two bits of |connection_id|, or
// empty if |connection_id| is empty, or the first two bits of the first byte
diff --git a/quiche/quic/load_balancer/load_balancer_decoder_test.cc b/quiche/quic/load_balancer/load_balancer_decoder_test.cc
index 1248db7..2f83672 100644
--- a/quiche/quic/load_balancer/load_balancer_decoder_test.cc
+++ b/quiche/quic/load_balancer/load_balancer_decoder_test.cc
@@ -26,8 +26,7 @@
// enough or |length| is valid for a server ID.
inline LoadBalancerServerId MakeServerId(const uint8_t array[],
const uint8_t length) {
- return *LoadBalancerServerId::Create(
- absl::Span<const uint8_t>(array, length));
+ return LoadBalancerServerId(absl::Span<const uint8_t>(array, length));
}
constexpr char kRawKey[] = {0x8f, 0x95, 0xf0, 0x92, 0x45, 0x76, 0x5f, 0x80,
@@ -103,19 +102,19 @@
}
TEST_F(LoadBalancerDecoderTest, NoServerIdEntry) {
- auto server_id = LoadBalancerServerId::Create({0x01, 0x02, 0x03});
- EXPECT_TRUE(server_id.has_value());
+ LoadBalancerServerId server_id({0x01, 0x02, 0x03});
+ EXPECT_TRUE(server_id.IsValid());
LoadBalancerDecoder decoder;
EXPECT_TRUE(
decoder.AddConfig(*LoadBalancerConfig::CreateUnencrypted(0, 3, 4)));
QuicConnectionId no_server_id_entry(
{0x00, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08});
- EXPECT_TRUE(decoder.GetServerId(no_server_id_entry).has_value());
+ EXPECT_TRUE(decoder.GetServerId(no_server_id_entry).IsValid());
}
TEST_F(LoadBalancerDecoderTest, InvalidConfigId) {
- auto server_id = LoadBalancerServerId::Create({0x01, 0x02, 0x03});
- EXPECT_TRUE(server_id.has_value());
+ LoadBalancerServerId server_id({0x01, 0x02, 0x03});
+ EXPECT_TRUE(server_id.IsValid());
LoadBalancerDecoder decoder;
EXPECT_TRUE(
decoder.AddConfig(*LoadBalancerConfig::CreateUnencrypted(1, 3, 4)));
@@ -124,50 +123,50 @@
EXPECT_FALSE(decoder
.GetServerId(QuicConnectionId(
{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07}))
- .has_value());
+ .IsValid());
}
TEST_F(LoadBalancerDecoderTest, UnroutableCodepoint) {
- auto server_id = LoadBalancerServerId::Create({0x01, 0x02, 0x03});
- EXPECT_TRUE(server_id.has_value());
+ LoadBalancerServerId server_id({0x01, 0x02, 0x03});
+ EXPECT_TRUE(server_id.IsValid());
LoadBalancerDecoder decoder;
EXPECT_TRUE(
decoder.AddConfig(*LoadBalancerConfig::CreateUnencrypted(1, 3, 4)));
EXPECT_FALSE(decoder
.GetServerId(QuicConnectionId(
{0xe0, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07}))
- .has_value());
+ .IsValid());
}
TEST_F(LoadBalancerDecoderTest, UnroutableCodepointAnyLength) {
- auto server_id = LoadBalancerServerId::Create({0x01, 0x02, 0x03});
- EXPECT_TRUE(server_id.has_value());
+ LoadBalancerServerId server_id({0x01, 0x02, 0x03});
+ EXPECT_TRUE(server_id.IsValid());
LoadBalancerDecoder decoder;
EXPECT_TRUE(
decoder.AddConfig(*LoadBalancerConfig::CreateUnencrypted(1, 3, 4)));
- EXPECT_FALSE(decoder.GetServerId(QuicConnectionId({0xff})).has_value());
+ EXPECT_FALSE(decoder.GetServerId(QuicConnectionId({0xff})).IsValid());
}
TEST_F(LoadBalancerDecoderTest, ConnectionIdTooShort) {
- auto server_id = LoadBalancerServerId::Create({0x01, 0x02, 0x03});
- EXPECT_TRUE(server_id.has_value());
+ LoadBalancerServerId server_id({0x01, 0x02, 0x03});
+ EXPECT_TRUE(server_id.IsValid());
LoadBalancerDecoder decoder;
EXPECT_TRUE(
decoder.AddConfig(*LoadBalancerConfig::CreateUnencrypted(0, 3, 4)));
EXPECT_FALSE(decoder
.GetServerId(QuicConnectionId(
{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06}))
- .has_value());
+ .IsValid());
}
TEST_F(LoadBalancerDecoderTest, ConnectionIdTooLongIsOK) {
- auto server_id = LoadBalancerServerId::Create({0x01, 0x02, 0x03});
+ LoadBalancerServerId server_id({0x01, 0x02, 0x03});
LoadBalancerDecoder decoder;
EXPECT_TRUE(
decoder.AddConfig(*LoadBalancerConfig::CreateUnencrypted(0, 3, 4)));
auto server_id_result = decoder.GetServerId(
QuicConnectionId({0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08}));
- EXPECT_TRUE(server_id_result.has_value());
+ EXPECT_TRUE(server_id_result.IsValid());
EXPECT_EQ(server_id_result, server_id);
}
@@ -180,7 +179,7 @@
EXPECT_TRUE(decoder
.GetServerId(QuicConnectionId(
{0x40, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07}))
- .has_value());
+ .IsValid());
}
TEST_F(LoadBalancerDecoderTest, DeleteConfigGoodId) {
@@ -190,14 +189,14 @@
EXPECT_FALSE(decoder
.GetServerId(QuicConnectionId(
{0x40, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07}))
- .has_value());
+ .IsValid());
}
// Create two server IDs and make sure the decoder decodes the correct one.
TEST_F(LoadBalancerDecoderTest, TwoServerIds) {
- auto server_id1 = LoadBalancerServerId::Create({0x01, 0x02, 0x03});
- EXPECT_TRUE(server_id1.has_value());
- auto server_id2 = LoadBalancerServerId::Create({0x04, 0x05, 0x06});
+ LoadBalancerServerId server_id1({0x01, 0x02, 0x03});
+ EXPECT_TRUE(server_id1.IsValid());
+ LoadBalancerServerId server_id2({0x04, 0x05, 0x06});
LoadBalancerDecoder decoder;
EXPECT_TRUE(
decoder.AddConfig(*LoadBalancerConfig::CreateUnencrypted(0, 3, 4)));
diff --git a/quiche/quic/load_balancer/load_balancer_encoder_test.cc b/quiche/quic/load_balancer/load_balancer_encoder_test.cc
index 929832b..c48de0d 100644
--- a/quiche/quic/load_balancer/load_balancer_encoder_test.cc
+++ b/quiche/quic/load_balancer/load_balancer_encoder_test.cc
@@ -116,8 +116,7 @@
// Convenience function to shorten the code. Does not check if |array| is long
// enough or |length| is valid for a server ID.
LoadBalancerServerId MakeServerId(const uint8_t array[], const uint8_t length) {
- return *LoadBalancerServerId::Create(
- absl::Span<const uint8_t>(array, length));
+ return LoadBalancerServerId(absl::Span<const uint8_t>(array, length));
}
constexpr char kRawKey[] = {0x8f, 0x95, 0xf0, 0x92, 0x45, 0x76, 0x5f, 0x80,
@@ -213,8 +212,8 @@
auto config = LoadBalancerConfig::Create(config_id, server_id_len, nonce_len,
absl::string_view(raw_key));
ASSERT_TRUE(config.has_value());
- EXPECT_TRUE(encoder->UpdateConfig(
- *config, *LoadBalancerServerId::Create(raw_server_id)));
+ EXPECT_TRUE(
+ encoder->UpdateConfig(*config, LoadBalancerServerId(raw_server_id)));
EXPECT_TRUE(encoder->IsEncoding());
const char raw_connection_id[] = {0x07, 0x8e, 0x9a, 0x91,
0xf4, 0x94, 0x97, 0x62};
diff --git a/quiche/quic/load_balancer/load_balancer_server_id.cc b/quiche/quic/load_balancer/load_balancer_server_id.cc
index 8702485..da95c92 100644
--- a/quiche/quic/load_balancer/load_balancer_server_id.cc
+++ b/quiche/quic/load_balancer/load_balancer_server_id.cc
@@ -17,15 +17,20 @@
namespace quic {
-std::optional<LoadBalancerServerId> LoadBalancerServerId::Create(
- const absl::Span<const uint8_t> data) {
+LoadBalancerServerId::LoadBalancerServerId(absl::string_view data)
+ : LoadBalancerServerId(absl::MakeSpan(
+ reinterpret_cast<const uint8_t*>(data.data()), data.length())) {}
+
+LoadBalancerServerId::LoadBalancerServerId(absl::Span<const uint8_t> data) {
if (data.length() == 0 || data.length() > kLoadBalancerMaxServerIdLen) {
QUIC_BUG(quic_bug_433312504_01)
<< "Attempted to create LoadBalancerServerId with length "
<< data.length();
- return std::optional<LoadBalancerServerId>();
+ length_ = 0;
+ return;
}
- return LoadBalancerServerId(data);
+ length_ = data.length();
+ memcpy(data_.data(), data.data(), data.length());
}
std::string LoadBalancerServerId::ToString() const {
@@ -33,9 +38,4 @@
absl::string_view(reinterpret_cast<const char*>(data_.data()), length_));
}
-LoadBalancerServerId::LoadBalancerServerId(const absl::Span<const uint8_t> data)
- : length_(data.length()) {
- memcpy(data_.data(), data.data(), data.length());
-}
-
} // namespace quic
diff --git a/quiche/quic/load_balancer/load_balancer_server_id.h b/quiche/quic/load_balancer/load_balancer_server_id.h
index 1a42f9c..a7b1d71 100644
--- a/quiche/quic/load_balancer/load_balancer_server_id.h
+++ b/quiche/quic/load_balancer/load_balancer_server_id.h
@@ -28,15 +28,12 @@
// (LoadBalancerServerId).
class QUIC_EXPORT_PRIVATE LoadBalancerServerId {
public:
- // Copies all the bytes from |data| into a new LoadBalancerServerId.
- static std::optional<LoadBalancerServerId> Create(
- absl::Span<const uint8_t> data);
+ // Creates an empty/invalid server id.
+ LoadBalancerServerId() : length_(0) {}
- // For callers with a string_view at hand.
- static std::optional<LoadBalancerServerId> Create(absl::string_view data) {
- return Create(absl::MakeSpan(reinterpret_cast<const uint8_t*>(data.data()),
- data.length()));
- }
+ // Copies all the bytes from |data| into a new LoadBalancerServerId.
+ explicit LoadBalancerServerId(absl::Span<const uint8_t> data);
+ explicit LoadBalancerServerId(absl::string_view data);
// Server IDs are opaque bytes, but defining these operators allows us to sort
// them into a tree and define ranges.
@@ -62,10 +59,10 @@
// Returns the server ID in hex format.
std::string ToString() const;
- private:
- // The constructor is private because it can't validate the input.
- LoadBalancerServerId(absl::Span<const uint8_t> data);
+ // Returns true if this is a valid server id.
+ bool IsValid() { return length_ != 0; }
+ private:
std::array<uint8_t, kLoadBalancerMaxServerIdLen> data_;
uint8_t length_;
};
diff --git a/quiche/quic/load_balancer/load_balancer_server_id_map_test.cc b/quiche/quic/load_balancer/load_balancer_server_id_map_test.cc
index 0b78ab5..5fb91f9 100644
--- a/quiche/quic/load_balancer/load_balancer_server_id_map_test.cc
+++ b/quiche/quic/load_balancer/load_balancer_server_id_map_test.cc
@@ -22,10 +22,9 @@
class LoadBalancerServerIdMapTest : public QuicTest {
public:
- const LoadBalancerServerId valid_server_id_ =
- *LoadBalancerServerId::Create(kServerId);
+ const LoadBalancerServerId valid_server_id_ = LoadBalancerServerId(kServerId);
const LoadBalancerServerId invalid_server_id_ =
- *LoadBalancerServerId::Create(absl::Span<const uint8_t>(kServerId, 3));
+ LoadBalancerServerId(absl::Span<const uint8_t>(kServerId, 3));
};
TEST_F(LoadBalancerServerIdMapTest, CreateWithBadServerIdLength) {
@@ -66,17 +65,17 @@
int record1 = 1, record2 = 2;
auto pool = LoadBalancerServerIdMap<int>::Create(4);
EXPECT_NE(pool, nullptr);
- auto other_server_id = LoadBalancerServerId::Create({0x01, 0x02, 0x03, 0x04});
- EXPECT_TRUE(other_server_id.has_value());
+ LoadBalancerServerId other_server_id({0x01, 0x02, 0x03, 0x04});
+ EXPECT_TRUE(other_server_id.IsValid());
pool->AddOrReplace(valid_server_id_, record1);
- pool->AddOrReplace(*other_server_id, record2);
+ pool->AddOrReplace(other_server_id, record2);
std::optional<int> result = pool->Lookup(valid_server_id_);
EXPECT_TRUE(result.has_value());
EXPECT_EQ(*result, record1);
auto result_ptr = pool->LookupNoCopy(valid_server_id_);
EXPECT_NE(result_ptr, nullptr);
EXPECT_EQ(*result_ptr, record1);
- result = pool->Lookup(*other_server_id);
+ result = pool->Lookup(other_server_id);
EXPECT_TRUE(result.has_value());
EXPECT_EQ(*result, record2);
}
diff --git a/quiche/quic/load_balancer/load_balancer_server_id_test.cc b/quiche/quic/load_balancer/load_balancer_server_id_test.cc
index faed01a..a882795 100644
--- a/quiche/quic/load_balancer/load_balancer_server_id_test.cc
+++ b/quiche/quic/load_balancer/load_balancer_server_id_test.cc
@@ -26,25 +26,23 @@
0x0c, 0x0d, 0x0e, 0x0f};
TEST_F(LoadBalancerServerIdTest, CreateReturnsNullIfTooLong) {
- EXPECT_QUIC_BUG(EXPECT_FALSE(LoadBalancerServerId::Create(
+ EXPECT_QUIC_BUG(EXPECT_FALSE(LoadBalancerServerId(
absl::Span<const uint8_t>(kRawServerId, 16))
- .has_value()),
+ .IsValid()),
"Attempted to create LoadBalancerServerId with length 16");
EXPECT_QUIC_BUG(
- EXPECT_FALSE(LoadBalancerServerId::Create(absl::Span<const uint8_t>())
- .has_value()),
+ EXPECT_FALSE(LoadBalancerServerId(absl::Span<const uint8_t>()).IsValid()),
"Attempted to create LoadBalancerServerId with length 0");
}
TEST_F(LoadBalancerServerIdTest, CompareIdenticalExceptLength) {
- auto server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 15));
- ASSERT_TRUE(server_id.has_value());
- EXPECT_EQ(server_id->length(), 15);
- auto shorter_server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 5));
- ASSERT_TRUE(shorter_server_id.has_value());
- EXPECT_EQ(shorter_server_id->length(), 5);
+ LoadBalancerServerId server_id(absl::Span<const uint8_t>(kRawServerId, 15));
+ ASSERT_TRUE(server_id.IsValid());
+ EXPECT_EQ(server_id.length(), 15);
+ LoadBalancerServerId shorter_server_id(
+ absl::Span<const uint8_t>(kRawServerId, 5));
+ ASSERT_TRUE(shorter_server_id.IsValid());
+ EXPECT_EQ(shorter_server_id.length(), 5);
// Shorter comes before longer if all bits match
EXPECT_TRUE(shorter_server_id < server_id);
EXPECT_FALSE(server_id < shorter_server_id);
@@ -53,52 +51,47 @@
}
TEST_F(LoadBalancerServerIdTest, AccessorFunctions) {
- auto server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 5));
- EXPECT_TRUE(server_id.has_value());
- EXPECT_EQ(server_id->length(), 5);
- EXPECT_EQ(memcmp(server_id->data().data(), kRawServerId, 5), 0);
- EXPECT_EQ(server_id->ToString(), "0001020304");
+ LoadBalancerServerId server_id(absl::Span<const uint8_t>(kRawServerId, 5));
+ EXPECT_TRUE(server_id.IsValid());
+ EXPECT_EQ(server_id.length(), 5);
+ EXPECT_EQ(memcmp(server_id.data().data(), kRawServerId, 5), 0);
+ EXPECT_EQ(server_id.ToString(), "0001020304");
}
TEST_F(LoadBalancerServerIdTest, CompareDifferentServerIds) {
- auto server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 5));
- ASSERT_TRUE(server_id.has_value());
- auto reverse = LoadBalancerServerId::Create({0x0f, 0x0e, 0x0d, 0x0c, 0x0b});
- ASSERT_TRUE(reverse.has_value());
+ LoadBalancerServerId server_id(absl::Span<const uint8_t>(kRawServerId, 5));
+ ASSERT_TRUE(server_id.IsValid());
+ LoadBalancerServerId reverse({0x0f, 0x0e, 0x0d, 0x0c, 0x0b});
+ ASSERT_TRUE(reverse.IsValid());
EXPECT_TRUE(server_id < reverse);
- auto long_server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 15));
+ LoadBalancerServerId long_server_id(
+ absl::Span<const uint8_t>(kRawServerId, 15));
EXPECT_TRUE(long_server_id < reverse);
}
TEST_F(LoadBalancerServerIdTest, EqualityOperators) {
- auto server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 15));
- ASSERT_TRUE(server_id.has_value());
- auto shorter_server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 5));
- ASSERT_TRUE(shorter_server_id.has_value());
+ LoadBalancerServerId server_id(absl::Span<const uint8_t>(kRawServerId, 15));
+ ASSERT_TRUE(server_id.IsValid());
+ LoadBalancerServerId shorter_server_id(
+ absl::Span<const uint8_t>(kRawServerId, 5));
+ ASSERT_TRUE(shorter_server_id.IsValid());
EXPECT_FALSE(server_id == shorter_server_id);
- auto server_id2 = server_id;
+ LoadBalancerServerId server_id2 = server_id;
EXPECT_TRUE(server_id == server_id2);
}
TEST_F(LoadBalancerServerIdTest, SupportsHash) {
- auto server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 15));
- ASSERT_TRUE(server_id.has_value());
- auto shorter_server_id =
- LoadBalancerServerId::Create(absl::Span<const uint8_t>(kRawServerId, 5));
- ASSERT_TRUE(shorter_server_id.has_value());
- auto different_server_id =
- LoadBalancerServerId::Create({0x0f, 0x0e, 0x0d, 0x0c, 0x0b});
- ASSERT_TRUE(different_server_id.has_value());
+ LoadBalancerServerId server_id(absl::Span<const uint8_t>(kRawServerId, 15));
+ ASSERT_TRUE(server_id.IsValid());
+ LoadBalancerServerId shorter_server_id(
+ absl::Span<const uint8_t>(kRawServerId, 5));
+ ASSERT_TRUE(shorter_server_id.IsValid());
+ LoadBalancerServerId different_server_id({0x0f, 0x0e, 0x0d, 0x0c, 0x0b});
+ ASSERT_TRUE(different_server_id.IsValid());
EXPECT_TRUE(absl::VerifyTypeImplementsAbslHashCorrectly({
- *server_id,
- *shorter_server_id,
- *different_server_id,
+ server_id,
+ shorter_server_id,
+ different_server_id,
}));
}
