Skip to content

Commit 8040f2a

Browse files
authored
MINOR: [C++] Avoid multimap::find unspecified behavior (#48607)
### Rationale for this change When a `std::multimap` has multiple entries with the same key, calling `m.find(key)` returns an unspecified element. Historically, this returns the first matching element. However, this is not guaranteed, and recent libc++ changes make this return an arbitrary element. This can lead to surprising behavior, i.e. different values returned based on what other entries are in the multimap (and what the shape of the internal tree is). ### What changes are included in this PR? Replace `m.find(key)` with `m.equal_range(key)` to preserve behavior and have predictable results. The behavior of this is guaranteed to return a range of all matching elements in insertion order, and the beginning of the range is the same element as what's normally returned by `m.find(key)`. ### Are these changes tested? Yes ### Are there any user-facing changes? No Authored-by: Jordan Rupprecht <[email protected]> Signed-off-by: David Li <[email protected]>
1 parent de6eb89 commit 8040f2a

File tree

3 files changed

+24
-27
lines changed

3 files changed

+24
-27
lines changed

cpp/src/arrow/flight/transport/grpc/grpc_client.cc

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include <sstream>
2626
#include <string>
2727
#include <thread>
28+
#include <tuple>
2829
#include <unordered_map>
2930
#include <utility>
3031

@@ -448,10 +449,11 @@ arrow::Result<std::pair<std::string, std::string>> GetBearerTokenHeader(
448449
// Get the auth token if it exists, this can be in the initial or the trailing metadata.
449450
auto trailing_headers = context.GetServerTrailingMetadata();
450451
auto initial_headers = context.GetServerInitialMetadata();
451-
auto bearer_iter = trailing_headers.find(internal::kAuthHeader);
452-
if (bearer_iter == trailing_headers.end()) {
453-
bearer_iter = initial_headers.find(internal::kAuthHeader);
454-
if (bearer_iter == initial_headers.end()) {
452+
auto [bearer_iter, bearer_end] = trailing_headers.equal_range(internal::kAuthHeader);
453+
if (bearer_iter == bearer_end) {
454+
std::tie(bearer_iter, bearer_end) =
455+
initial_headers.equal_range(internal::kAuthHeader);
456+
if (bearer_iter == bearer_end) {
455457
return std::make_pair("", "");
456458
}
457459
}

cpp/src/arrow/flight/transport/grpc/grpc_server.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -307,9 +307,10 @@ class GrpcServiceHandler final : public FlightService::Service {
307307
}
308308
} else {
309309
const auto client_metadata = context->client_metadata();
310-
const auto auth_header = client_metadata.find(kGrpcAuthHeader);
310+
const auto [auth_header, auth_header_end] =
311+
client_metadata.equal_range(kGrpcAuthHeader);
311312
std::string token;
312-
if (auth_header == client_metadata.end()) {
313+
if (auth_header == auth_header_end) {
313314
token = "";
314315
} else {
315316
token = std::string(auth_header->second.data(), auth_header->second.length());

cpp/src/arrow/flight/transport/grpc/util_internal.cc

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -55,31 +55,25 @@ static bool FromGrpcContext(const ::grpc::ClientContext& ctx,
5555
const std::multimap<::grpc::string_ref, ::grpc::string_ref>& trailers =
5656
ctx.GetServerTrailingMetadata();
5757

58-
const auto code_val = trailers.find(kGrpcStatusCodeHeader);
59-
if (code_val == trailers.end()) return false;
58+
const auto [code_val_begin, code_val_end] = trailers.equal_range(kGrpcStatusCodeHeader);
59+
if (code_val_begin == code_val_end) return false;
6060

61-
const auto message_val = trailers.find(kGrpcStatusMessageHeader);
62-
const std::optional<std::string> message =
63-
message_val == trailers.end()
64-
? std::nullopt
65-
: std::optional<std::string>(
66-
std::string(message_val->second.data(), message_val->second.size()));
61+
std::optional<std::string> message;
62+
if (const auto [it, end] = trailers.equal_range(kGrpcStatusMessageHeader); it != end) {
63+
message = std::string(it->second.data(), it->second.size());
64+
}
6765

68-
const auto detail_val = trailers.find(kGrpcStatusDetailHeader);
69-
const std::optional<std::string> detail_message =
70-
detail_val == trailers.end()
71-
? std::nullopt
72-
: std::optional<std::string>(
73-
std::string(detail_val->second.data(), detail_val->second.size()));
66+
std::optional<std::string> detail_message;
67+
if (const auto [it, end] = trailers.equal_range(kGrpcStatusDetailHeader); it != end) {
68+
detail_message = std::string(it->second.data(), it->second.size());
69+
}
7470

75-
const auto grpc_detail_val = trailers.find(kBinaryErrorDetailsKey);
76-
const std::optional<std::string> detail_bin =
77-
grpc_detail_val == trailers.end()
78-
? std::nullopt
79-
: std::optional<std::string>(std::string(grpc_detail_val->second.data(),
80-
grpc_detail_val->second.size()));
71+
std::optional<std::string> detail_bin;
72+
if (const auto [it, end] = trailers.equal_range(kBinaryErrorDetailsKey); it != end) {
73+
detail_bin = std::string(it->second.data(), it->second.size());
74+
}
8175

82-
std::string code_str(code_val->second.data(), code_val->second.size());
76+
std::string code_str(code_val_begin->second.data(), code_val_begin->second.size());
8377
*status = internal::ReconstructStatus(code_str, current_status, std::move(message),
8478
std::move(detail_message), std::move(detail_bin),
8579
std::move(flight_status_detail));

0 commit comments

Comments
 (0)