Summary
Fixes a bug in google::cloud::internal::GroupOptions(Options& bundle, Tail&&... t) (google/cloud/internal/group_options.h) where passing a non-const lvalue google::cloud::Options variable silently moved out of the caller's Options object on the first call, leaving it empty for subsequent calls.
Root Cause
In google/cloud/internal/group_options.h, the non-const lvalue reference overload was defined as:
template <typename... Tail>
Options GroupOptions(Options& bundle, Tail&&... t) {
return google::cloud::internal::MergeOptions(
GroupOptions(std::forward<Tail>(t)...), std::move(bundle));
}Because MergeOptions(Options a, Options b) accepts its parameters by value, std::move(bundle) invokes Options(Options&&) on the caller's lvalue bundle, stealing its internal std::unordered_map (m_) and leaving the caller's variable empty.
Minimal PoC & Before/After Output
#include "google/cloud/internal/group_options.h"
#include "google/cloud/options.h"
#include <chrono>
#include <iostream>
struct TimeoutOption {
using Type = std::chrono::seconds;
};
// Mirrors `storage::Client::SpanOptions` in `google/cloud/storage/client.h`:
template <typename... RequestOptions>
void SimulateGetObjectMetadata(int call_num,
google::cloud::Options const& client_defaults,
RequestOptions&&... o) {
google::cloud::Options merged = google::cloud::internal::GroupOptions(
client_defaults, std::forward<RequestOptions>(o)...);
std::cout << " Call #" << call_num
<< " -> effective TimeoutOption = "
<< merged.get<TimeoutOption>().count() << "s\n";
}
int main() {
google::cloud::Options const client_defaults =
google::cloud::Options{}.set<TimeoutOption>(std::chrono::seconds(120));
// Non-const lvalue Options variable with a 30s per-call override:
google::cloud::Options per_call_opts =
google::cloud::Options{}.set<TimeoutOption>(std::chrono::seconds(30));
for (int i = 1; i <= 3; ++i) {
SimulateGetObjectMetadata(i, client_defaults, per_call_opts);
std::cout << " After Call #" << i
<< ": per_call_opts.has<TimeoutOption>() = " << std::boolalpha
<< per_call_opts.has<TimeoutOption>() << "\n";
}
}Before this PR (origin/main):
Call #1 -> effective TimeoutOption = 30s
After Call #1: per_call_opts.has<TimeoutOption>() = false
Call #2 -> effective TimeoutOption = 120s
After Call #2: per_call_opts.has<TimeoutOption>() = false
Call #3 -> effective TimeoutOption = 120s
After Call #3: per_call_opts.has<TimeoutOption>() = false
After this PR:
Call #1 -> effective TimeoutOption = 30s
After Call #1: per_call_opts.has<TimeoutOption>() = true
Call #2 -> effective TimeoutOption = 30s
After Call #2: per_call_opts.has<TimeoutOption>() = true
Call #3 -> effective TimeoutOption = 30s
After Call #3: per_call_opts.has<TimeoutOption>() = true
Fix
Pass bundle without std::move in GroupOptions(Options& bundle, Tail&&... t) so that non-const lvalue Options objects are copied, while preserving std::move(bundle) in the rvalue GroupOptions(Options&& bundle, Tail&&... t) overload.
Testing
- Added
MakeOptionSpanTest.NonConstLvalueOptionsPreserved in google/cloud/internal/group_options_test.cc.