Second attempt: client channel: don't hold mutexes while calling the ConfigSelector or the LB picker (#32326)

Original attempt was #31973, reverted in #32324 due to test flakiness.

There were two problems causing test flakiness here.

The first problem was that, upon resolver error, we were dispatching an
async callback to re-process each of the queued picks *before* we
updated the channel's connectivity state, which meant that the queued
picks might be re-processed in another thread before the new
connectivity state was set, so tests that expected the state to be
TRANSIENT_FAILURE once RPCs failed might not see the expected state.

The second problem affected the xDS ring hash tests, and it's a bit more
involved to explain.

We have an e2e test that simulates an aggregate cluster failover from a
primary cluster using ring_hash at startup. The primary cluster has two
addresses, both of which are unreachable when the client starts up, so
the client should immediately fail over to the secondary cluster, which
does have reachable endpoints. The test requires that no RPCs are failed
while this failover occurs. The original PR made this test flaky.

The problem here was caused by a combination of two factors:

1. Prior to the original PR, when the picker was updated (which happens
inside the WorkSerializer), we re-processed previously queued picks
synchronously, so it was not possible for another subchannel
connectivity state update (which also happens in the WorkSerializer) to
be processed between the time that we updated the picker and the time
that we re-processed the previously queued picks. The original PR
changed this such that the queued picks are re-processed asynchronously
(outside of the WorkSerializer), so it is now possible for a subchannel
connectivity state update to be processed between when the picker is
updated and when we re-process the previously queued picks.

2. Unlike most LB policies, where the picker does not see updated
subchannel connectivity states until a new picker is created, the
ring_hash picker gets the subchannel connectivity states from the LB
policy via a lock, so it can wind up seeing the new states before it
gets updated. This means that when a subchannel connectivity state
update is processed by the ring_hash policy in the WorkSerializer, it
will immediately be seen by the existing picker, even without a picker
update.

With those two points in mind, the sequence of events in the failing
test were as follows:

1. The pick is attempted in the ring_hash picker for the primary
cluster. This causes the first subchannel to attempt to connect.
2. The subchannel transitions from IDLE to CONNECTING. A new picker is
returned due to the subchannel connectivity state change, and the
channel retries the queued pick. The retried pick is done
asynchronously, but in this case it does not matter: the call will be
re-queued.
3. The connection attempt fails, and the subchannel reports
TRANSIENT_FAILURE. A new picker is again returned, and the channel
retries the queued pick. The retried pick is done asynchronously, but in
this case it does not matter: this causes the picker to trigger a
connection attempt for the second subchannel.
4. The second subchannel transitions from IDLE to CONNECTING. A new
picker is again returned, and the channel retries the queued pick. The
retried pick is done asynchronously, and in this case it *does* matter.
5. The second subchannel now transitions to TRANSIENT_FAILURE. The
ring_hash policy will now report TRANSIENT_FAILURE, but before it can
finish that...
6. ...In another thread, the channel now tries to re-process the queued
pick using the CONNECTING picker from step 4. However, because the
ring_hash policy has already seen the TRANSIENT_FAILURE report from the
second subchannel, that picker will now fail the pick instead of queuing
it.

After discussion with @ejona86 and @dfawley (since this bug actually
exists in Java and Go as well), we agreed that the right solution is to
change the ring_hash picker to contain its own copy of the subchannel
connectivity state information, rather than sharing that information
with the LB policy using synchronization.
pull/32223/head^2
Mark D. Roth 2 years ago committed by GitHub
parent 6589340efc
commit 8249fc10a9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 1
      BUILD
  2. 6
      src/core/BUILD
  3. 781
      src/core/ext/filters/client_channel/client_channel.cc
  4. 85
      src/core/ext/filters/client_channel/client_channel.h
  5. 17
      src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
  6. 280
      src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc
  7. 14
      src/core/ext/filters/client_channel/lb_policy/rls/rls.cc
  8. 24
      src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
  9. 14
      src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc
  10. 3
      src/core/ext/filters/client_channel/retry_filter.cc
  11. 7
      src/core/ext/xds/xds_endpoint.cc
  12. 10
      src/core/ext/xds/xds_endpoint.h
  13. 22
      src/core/lib/load_balancing/lb_policy.cc
  14. 6
      src/core/lib/load_balancing/lb_policy.h

@ -2785,6 +2785,7 @@ grpc_cc_library(
],
external_deps = [
"absl/base:core_headers",
"absl/container:flat_hash_set",
"absl/container:inlined_vector",
"absl/status",
"absl/status:statusor",

@ -2495,6 +2495,7 @@ grpc_cc_library(
srcs = ["lib/load_balancing/lb_policy.cc"],
hdrs = ["lib/load_balancing/lb_policy.h"],
external_deps = [
"absl/base:core_headers",
"absl/status",
"absl/status:statusor",
"absl/strings",
@ -2514,6 +2515,7 @@ grpc_cc_library(
"//:debug_location",
"//:event_engine_base_hdrs",
"//:exec_ctx",
"//:gpr",
"//:gpr_platform",
"//:grpc_trace",
"//:orphanable",
@ -3849,6 +3851,7 @@ grpc_cc_library(
"absl/base:core_headers",
"absl/functional:bind_front",
"absl/memory",
"absl/random",
"absl/status",
"absl/status:statusor",
"absl/strings",
@ -4421,6 +4424,7 @@ grpc_cc_library(
"json_object_loader",
"lb_policy",
"lb_policy_factory",
"ref_counted",
"subchannel_interface",
"unique_type_name",
"validation_errors",
@ -4445,6 +4449,7 @@ grpc_cc_library(
"ext/filters/client_channel/lb_policy/round_robin/round_robin.cc",
],
external_deps = [
"absl/random",
"absl/status",
"absl/status:statusor",
"absl/strings",
@ -4631,6 +4636,7 @@ grpc_cc_library(
"ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc",
],
external_deps = [
"absl/base:core_headers",
"absl/random",
"absl/status",
"absl/status:statusor",

File diff suppressed because it is too large Load Diff

@ -24,11 +24,11 @@
#include <atomic>
#include <map>
#include <memory>
#include <set>
#include <string>
#include <utility>
#include "absl/base/thread_annotations.h"
#include "absl/container/flat_hash_set.h"
#include "absl/status/status.h"
#include "absl/strings/string_view.h"
#include "absl/types/optional.h"
@ -222,15 +222,6 @@ class ClientChannel {
std::atomic<bool> done_{false};
};
struct ResolverQueuedCall {
grpc_call_element* elem;
ResolverQueuedCall* next = nullptr;
};
struct LbQueuedCall {
LoadBalancedCall* lb_call;
LbQueuedCall* next = nullptr;
};
ClientChannel(grpc_channel_element_args* args, grpc_error_handle* error);
~ClientChannel();
@ -246,6 +237,9 @@ class ClientChannel {
// Note: All methods with "Locked" suffix must be invoked from within
// work_serializer_.
void ReprocessQueuedResolverCalls()
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&resolution_mu_);
void OnResolverResultChangedLocked(Resolver::Result result)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(*work_serializer_);
void OnResolverErrorLocked(absl::Status status)
@ -284,20 +278,6 @@ class ClientChannel {
void TryToConnectLocked() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*work_serializer_);
// These methods all require holding resolution_mu_.
void AddResolverQueuedCall(ResolverQueuedCall* call,
grpc_polling_entity* pollent)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(resolution_mu_);
void RemoveResolverQueuedCall(ResolverQueuedCall* to_remove,
grpc_polling_entity* pollent)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(resolution_mu_);
// These methods all require holding data_plane_mu_.
void AddLbQueuedCall(LbQueuedCall* call, grpc_polling_entity* pollent)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(data_plane_mu_);
void RemoveLbQueuedCall(LbQueuedCall* to_remove, grpc_polling_entity* pollent)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(data_plane_mu_);
//
// Fields set at construction and never modified.
//
@ -316,9 +296,9 @@ class ClientChannel {
// Fields related to name resolution. Guarded by resolution_mu_.
//
mutable Mutex resolution_mu_;
// Linked list of calls queued waiting for resolver result.
ResolverQueuedCall* resolver_queued_calls_ ABSL_GUARDED_BY(resolution_mu_) =
nullptr;
// List of calls queued waiting for resolver result.
absl::flat_hash_set<grpc_call_element*> resolver_queued_calls_
ABSL_GUARDED_BY(resolution_mu_);
// Data from service config.
absl::Status resolver_transient_failure_error_
ABSL_GUARDED_BY(resolution_mu_);
@ -330,13 +310,13 @@ class ClientChannel {
ABSL_GUARDED_BY(resolution_mu_);
//
// Fields used in the data plane. Guarded by data_plane_mu_.
// Fields related to LB picks. Guarded by lb_mu_.
//
mutable Mutex data_plane_mu_;
mutable Mutex lb_mu_;
RefCountedPtr<LoadBalancingPolicy::SubchannelPicker> picker_
ABSL_GUARDED_BY(data_plane_mu_);
// Linked list of calls queued waiting for LB pick.
LbQueuedCall* lb_queued_calls_ ABSL_GUARDED_BY(data_plane_mu_) = nullptr;
ABSL_GUARDED_BY(lb_mu_);
absl::flat_hash_set<LoadBalancedCall*> lb_queued_calls_
ABSL_GUARDED_BY(lb_mu_);
//
// Fields used in the control plane. Guarded by work_serializer.
@ -360,7 +340,7 @@ class ClientChannel {
// The set of SubchannelWrappers that currently exist.
// No need to hold a ref, since the map is updated in the control-plane
// work_serializer when the SubchannelWrappers are created and destroyed.
std::set<SubchannelWrapper*> subchannel_wrappers_
absl::flat_hash_set<SubchannelWrapper*> subchannel_wrappers_
ABSL_GUARDED_BY(*work_serializer_);
int keepalive_time_ ABSL_GUARDED_BY(*work_serializer_) = -1;
grpc_error_handle disconnect_error_ ABSL_GUARDED_BY(*work_serializer_);
@ -422,16 +402,11 @@ class ClientChannel::LoadBalancedCall
void StartTransportStreamOpBatch(grpc_transport_stream_op_batch* batch);
// Invoked by channel for queued LB picks when the picker is updated.
static void PickSubchannel(void* arg, grpc_error_handle error);
// Helper function for performing an LB pick while holding the data plane
// mutex. Returns true if the pick is complete, in which case the caller
// must invoke PickDone() or AsyncPickDone() with the returned error.
bool PickSubchannelLocked(grpc_error_handle* error)
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&ClientChannel::data_plane_mu_);
// Schedules a callback to process the completed pick. The callback
// will not run until after this method returns.
void AsyncPickDone(grpc_error_handle error);
void PickSubchannel(bool was_queued);
// Called by channel when removing a call from the list of queued calls.
void RemoveCallFromLbQueuedCallsLocked()
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&ClientChannel::lb_mu_);
RefCountedPtr<SubchannelCall> subchannel_call() const {
return subchannel_call_;
@ -479,14 +454,14 @@ class ClientChannel::LoadBalancedCall
void RecordCallCompletion(absl::Status status);
void CreateSubchannelCall();
// Invoked when a pick is completed, on both success or failure.
static void PickDone(void* arg, grpc_error_handle error);
// Removes the call from the channel's list of queued picks if present.
void MaybeRemoveCallFromLbQueuedCallsLocked()
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&ClientChannel::data_plane_mu_);
// Helper function for performing an LB pick with a specified picker.
// Returns true if the pick is complete.
bool PickSubchannelImpl(LoadBalancingPolicy::SubchannelPicker* picker,
grpc_error_handle* error);
// Adds the call to the channel's list of queued picks if not already present.
void MaybeAddCallToLbQueuedCallsLocked()
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&ClientChannel::data_plane_mu_);
void AddCallToLbQueuedCallsLocked()
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&ClientChannel::lb_mu_);
ClientChannel* chand_;
@ -513,15 +488,9 @@ class ClientChannel::LoadBalancedCall
// Set when we fail inside the LB call.
grpc_error_handle failure_error_;
grpc_closure pick_closure_;
// Accessed while holding ClientChannel::data_plane_mu_.
ClientChannel::LbQueuedCall queued_call_
ABSL_GUARDED_BY(&ClientChannel::data_plane_mu_);
bool queued_pending_lb_pick_ ABSL_GUARDED_BY(&ClientChannel::data_plane_mu_) =
false;
// Accessed while holding ClientChannel::lb_mu_.
LbQueuedCallCanceller* lb_call_canceller_
ABSL_GUARDED_BY(&ClientChannel::data_plane_mu_) = nullptr;
ABSL_GUARDED_BY(&ClientChannel::lb_mu_) = nullptr;
RefCountedPtr<ConnectedSubchannel> connected_subchannel_;
const BackendMetricData* backend_metric_data_ = nullptr;

@ -61,6 +61,7 @@
#include <string.h>
#include <algorithm>
#include <atomic>
#include <initializer_list>
#include <map>
#include <memory>
@ -389,19 +390,15 @@ class GrpcLb : public LoadBalancingPolicy {
// Returns the LB token to use for a drop, or null if the call
// should not be dropped.
//
// Note: This is called from the picker, so it will be invoked in
// the channel's data plane mutex, NOT the control plane
// work_serializer. It should not be accessed by any other part of the LB
// policy.
// Note: This is called from the picker, NOT from inside the control
// plane work_serializer.
const char* ShouldDrop();
private:
std::vector<GrpcLbServer> serverlist_;
// Guarded by the channel's data plane mutex, NOT the control
// plane work_serializer. It should not be accessed by anything but the
// picker via the ShouldDrop() method.
size_t drop_index_ = 0;
// Accessed from the picker, so needs synchronization.
std::atomic<size_t> drop_index_{0};
};
class Picker : public SubchannelPicker {
@ -717,8 +714,8 @@ bool GrpcLb::Serverlist::ContainsAllDropEntries() const {
const char* GrpcLb::Serverlist::ShouldDrop() {
if (serverlist_.empty()) return nullptr;
GrpcLbServer& server = serverlist_[drop_index_];
drop_index_ = (drop_index_ + 1) % serverlist_.size();
size_t index = drop_index_.fetch_add(1, std::memory_order_relaxed);
GrpcLbServer& server = serverlist_[index % serverlist_.size()];
return server.drop ? server.load_balance_token : nullptr;
}

@ -22,7 +22,6 @@
#include <stdlib.h>
#include <algorithm>
#include <atomic>
#include <cmath>
#include <memory>
#include <string>
@ -30,7 +29,6 @@
#include <vector>
#include "absl/base/attributes.h"
#include "absl/base/thread_annotations.h"
#include "absl/container/inlined_vector.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
@ -39,11 +37,10 @@
#include "absl/strings/string_view.h"
#include "absl/types/optional.h"
#include <grpc/grpc.h>
#define XXH_INLINE_ALL
#include "xxhash.h"
#include <grpc/grpc.h>
#include <grpc/impl/connectivity_state.h>
#include <grpc/support/log.h>
@ -55,8 +52,8 @@
#include "src/core/lib/debug/trace.h"
#include "src/core/lib/gprpp/debug_location.h"
#include "src/core/lib/gprpp/orphanable.h"
#include "src/core/lib/gprpp/ref_counted.h"
#include "src/core/lib/gprpp/ref_counted_ptr.h"
#include "src/core/lib/gprpp/sync.h"
#include "src/core/lib/gprpp/unique_type_name.h"
#include "src/core/lib/gprpp/work_serializer.h"
#include "src/core/lib/iomgr/closure.h"
@ -143,8 +140,6 @@ class RingHash : public LoadBalancingPolicy {
void ResetBackoffLocked() override;
private:
~RingHash() override;
// Forward declaration.
class RingHashSubchannelList;
@ -165,13 +160,11 @@ class RingHash : public LoadBalancingPolicy {
const ServerAddress& address() const { return address_; }
grpc_connectivity_state GetConnectivityState() const {
return connectivity_state_.load(std::memory_order_relaxed);
grpc_connectivity_state logical_connectivity_state() const {
return logical_connectivity_state_;
}
absl::Status GetConnectivityStatus() const {
MutexLock lock(&mu_);
return connectivity_status_;
const absl::Status& logical_connectivity_status() const {
return logical_connectivity_status_;
}
private:
@ -188,20 +181,28 @@ class RingHash : public LoadBalancingPolicy {
// subchannel in some cases; for example, once this is set to
// TRANSIENT_FAILURE, we do not change it again until we get READY,
// so we skip any interim stops in CONNECTING.
// Uses an atomic so that it can be accessed outside of the WorkSerializer.
std::atomic<grpc_connectivity_state> connectivity_state_{GRPC_CHANNEL_IDLE};
mutable Mutex mu_;
absl::Status connectivity_status_ ABSL_GUARDED_BY(&mu_);
grpc_connectivity_state logical_connectivity_state_ = GRPC_CHANNEL_IDLE;
absl::Status logical_connectivity_status_;
};
// A list of subchannels and the ring containing those subchannels.
class RingHashSubchannelList
: public SubchannelList<RingHashSubchannelList, RingHashSubchannelData> {
public:
struct RingEntry {
uint64_t hash;
RingHashSubchannelData* subchannel;
class Ring : public RefCounted<Ring> {
public:
struct RingEntry {
uint64_t hash;
size_t subchannel_index;
};
Ring(RingHashLbConfig* config, RingHashSubchannelList* subchannel_list,
const ChannelArgs& args);
const std::vector<RingEntry>& ring() const { return ring_; }
private:
std::vector<RingEntry> ring_;
};
RingHashSubchannelList(RingHash* policy, ServerAddressList addresses,
@ -212,7 +213,7 @@ class RingHash : public LoadBalancingPolicy {
p->Unref(DEBUG_LOCATION, "subchannel_list");
}
const std::vector<RingEntry>& ring() const { return ring_; }
RefCountedPtr<Ring> ring() { return ring_; }
// Updates the counters of subchannels in each state when a
// subchannel transitions from old_state to new_state.
@ -236,7 +237,7 @@ class RingHash : public LoadBalancingPolicy {
size_t num_connecting_ = 0;
size_t num_transient_failure_ = 0;
std::vector<RingEntry> ring_;
RefCountedPtr<Ring> ring_;
// The index of the subchannel currently doing an internally
// triggered connection attempt, if any.
@ -251,24 +252,31 @@ class RingHash : public LoadBalancingPolicy {
class Picker : public SubchannelPicker {
public:
explicit Picker(RefCountedPtr<RingHashSubchannelList> subchannel_list)
: subchannel_list_(std::move(subchannel_list)) {}
~Picker() override {
// Hop into WorkSerializer to unref the subchannel list, since that may
// trigger the unreffing of the underlying subchannels.
MakeOrphanable<WorkSerializerRunner>(std::move(subchannel_list_));
Picker(RefCountedPtr<RingHash> ring_hash_lb,
RingHashSubchannelList* subchannel_list)
: ring_hash_lb_(std::move(ring_hash_lb)),
ring_(subchannel_list->ring()) {
subchannels_.reserve(subchannel_list->num_subchannels());
for (size_t i = 0; i < subchannel_list->num_subchannels(); ++i) {
RingHashSubchannelData* subchannel_data =
subchannel_list->subchannel(i);
subchannels_.emplace_back(
SubchannelInfo{subchannel_data->subchannel()->Ref(),
subchannel_data->logical_connectivity_state(),
subchannel_data->logical_connectivity_status()});
}
}
PickResult Pick(PickArgs args) override;
private:
// An interface for running a callback in the control plane WorkSerializer.
class WorkSerializerRunner : public Orphanable {
// A fire-and-forget class that schedules subchannel connection attempts
// on the control plane WorkSerializer.
class SubchannelConnectionAttempter : public Orphanable {
public:
explicit WorkSerializerRunner(
RefCountedPtr<RingHashSubchannelList> subchannel_list)
: subchannel_list_(std::move(subchannel_list)) {
explicit SubchannelConnectionAttempter(
RefCountedPtr<RingHash> ring_hash_lb)
: ring_hash_lb_(std::move(ring_hash_lb)) {
GRPC_CLOSURE_INIT(&closure_, RunInExecCtx, this, nullptr);
}
@ -278,56 +286,43 @@ class RingHash : public LoadBalancingPolicy {
ExecCtx::Run(DEBUG_LOCATION, &closure_, absl::OkStatus());
}
// Will be invoked inside of the WorkSerializer.
virtual void Run() {}
protected:
RingHash* ring_hash_lb() const {
return static_cast<RingHash*>(subchannel_list_->policy());
void AddSubchannel(RefCountedPtr<SubchannelInterface> subchannel) {
subchannels_.push_back(std::move(subchannel));
}
private:
static void RunInExecCtx(void* arg, grpc_error_handle /*error*/) {
auto* self = static_cast<WorkSerializerRunner*>(arg);
self->ring_hash_lb()->work_serializer()->Run(
auto* self = static_cast<SubchannelConnectionAttempter*>(arg);
self->ring_hash_lb_->work_serializer()->Run(
[self]() {
self->Run();
if (!self->ring_hash_lb_->shutdown_) {
for (auto& subchannel : self->subchannels_) {
subchannel->RequestConnection();
}
}
delete self;
},
DEBUG_LOCATION);
}
RefCountedPtr<RingHashSubchannelList> subchannel_list_;
RefCountedPtr<RingHash> ring_hash_lb_;
grpc_closure closure_;
std::vector<RefCountedPtr<SubchannelInterface>> subchannels_;
};
// A fire-and-forget class that schedules subchannel connection attempts
// on the control plane WorkSerializer.
class SubchannelConnectionAttempter : public WorkSerializerRunner {
public:
explicit SubchannelConnectionAttempter(
RefCountedPtr<RingHashSubchannelList> subchannel_list)
: WorkSerializerRunner(std::move(subchannel_list)) {}
void AddSubchannel(RefCountedPtr<SubchannelInterface> subchannel) {
subchannels_.push_back(std::move(subchannel));
}
void Run() override {
if (!ring_hash_lb()->shutdown_) {
for (auto& subchannel : subchannels_) {
subchannel->RequestConnection();
}
}
}
private:
std::vector<RefCountedPtr<SubchannelInterface>> subchannels_;
struct SubchannelInfo {
RefCountedPtr<SubchannelInterface> subchannel;
grpc_connectivity_state state;
absl::Status status;
};
RefCountedPtr<RingHashSubchannelList> subchannel_list_;
RefCountedPtr<RingHash> ring_hash_lb_;
RefCountedPtr<RingHashSubchannelList::Ring> ring_;
std::vector<SubchannelInfo> subchannels_;
};
~RingHash() override;
void ShutdownLocked() override;
// Current config from resolver.
@ -353,7 +348,7 @@ RingHash::PickResult RingHash::Picker::Pick(PickArgs args) {
return PickResult::Fail(
absl::InternalError("ring hash value is not a number"));
}
const auto& ring = subchannel_list_->ring();
const auto& ring = ring_->ring();
// Ported from https://github.com/RJ/ketama/blob/master/libketama/ketama.c
// (ketama_get_server) NOTE: The algorithm depends on using signed integers
// for lowp, highp, and first_index. Do not change them!
@ -386,27 +381,25 @@ RingHash::PickResult RingHash::Picker::Pick(PickArgs args) {
[&](RefCountedPtr<SubchannelInterface> subchannel) {
if (subchannel_connection_attempter == nullptr) {
subchannel_connection_attempter =
MakeOrphanable<SubchannelConnectionAttempter>(
subchannel_list_->Ref(DEBUG_LOCATION,
"SubchannelConnectionAttempter"));
MakeOrphanable<SubchannelConnectionAttempter>(ring_hash_lb_->Ref(
DEBUG_LOCATION, "SubchannelConnectionAttempter"));
}
subchannel_connection_attempter->AddSubchannel(std::move(subchannel));
};
switch (ring[first_index].subchannel->GetConnectivityState()) {
SubchannelInfo& first_subchannel =
subchannels_[ring[first_index].subchannel_index];
switch (first_subchannel.state) {
case GRPC_CHANNEL_READY:
return PickResult::Complete(
ring[first_index].subchannel->subchannel()->Ref());
return PickResult::Complete(first_subchannel.subchannel);
case GRPC_CHANNEL_IDLE:
ScheduleSubchannelConnectionAttempt(
ring[first_index].subchannel->subchannel()->Ref());
ScheduleSubchannelConnectionAttempt(first_subchannel.subchannel);
ABSL_FALLTHROUGH_INTENDED;
case GRPC_CHANNEL_CONNECTING:
return PickResult::Queue();
default: // GRPC_CHANNEL_TRANSIENT_FAILURE
break;
}
ScheduleSubchannelConnectionAttempt(
ring[first_index].subchannel->subchannel()->Ref());
ScheduleSubchannelConnectionAttempt(first_subchannel.subchannel);
// Loop through remaining subchannels to find one in READY.
// On the way, we make sure the right set of connection attempts
// will happen.
@ -414,19 +407,17 @@ RingHash::PickResult RingHash::Picker::Pick(PickArgs args) {
bool found_first_non_failed = false;
for (size_t i = 1; i < ring.size(); ++i) {
const auto& entry = ring[(first_index + i) % ring.size()];
if (entry.subchannel == ring[first_index].subchannel) {
if (entry.subchannel_index == ring[first_index].subchannel_index) {
continue;
}
grpc_connectivity_state connectivity_state =
entry.subchannel->GetConnectivityState();
if (connectivity_state == GRPC_CHANNEL_READY) {
return PickResult::Complete(entry.subchannel->subchannel()->Ref());
SubchannelInfo& subchannel_info = subchannels_[entry.subchannel_index];
if (subchannel_info.state == GRPC_CHANNEL_READY) {
return PickResult::Complete(subchannel_info.subchannel);
}
if (!found_second_subchannel) {
switch (connectivity_state) {
switch (subchannel_info.state) {
case GRPC_CHANNEL_IDLE:
ScheduleSubchannelConnectionAttempt(
entry.subchannel->subchannel()->Ref());
ScheduleSubchannelConnectionAttempt(subchannel_info.subchannel);
ABSL_FALLTHROUGH_INTENDED;
case GRPC_CHANNEL_CONNECTING:
return PickResult::Queue();
@ -436,13 +427,11 @@ RingHash::PickResult RingHash::Picker::Pick(PickArgs args) {
found_second_subchannel = true;
}
if (!found_first_non_failed) {
if (connectivity_state == GRPC_CHANNEL_TRANSIENT_FAILURE) {
ScheduleSubchannelConnectionAttempt(
entry.subchannel->subchannel()->Ref());
if (subchannel_info.state == GRPC_CHANNEL_TRANSIENT_FAILURE) {
ScheduleSubchannelConnectionAttempt(subchannel_info.subchannel);
} else {
if (connectivity_state == GRPC_CHANNEL_IDLE) {
ScheduleSubchannelConnectionAttempt(
entry.subchannel->subchannel()->Ref());
if (subchannel_info.state == GRPC_CHANNEL_IDLE) {
ScheduleSubchannelConnectionAttempt(subchannel_info.subchannel);
}
found_first_non_failed = true;
}
@ -450,27 +439,16 @@ RingHash::PickResult RingHash::Picker::Pick(PickArgs args) {
}
return PickResult::Fail(absl::UnavailableError(absl::StrCat(
"ring hash cannot find a connected subchannel; first failure: ",
ring[first_index].subchannel->GetConnectivityStatus().ToString())));
first_subchannel.status.ToString())));
}
//
// RingHash::RingHashSubchannelList
// RingHash::RingHashSubchannelList::Ring
//
RingHash::RingHashSubchannelList::RingHashSubchannelList(
RingHash* policy, ServerAddressList addresses, const ChannelArgs& args)
: SubchannelList(policy,
(GRPC_TRACE_FLAG_ENABLED(grpc_lb_ring_hash_trace)
? "RingHashSubchannelList"
: nullptr),
std::move(addresses), policy->channel_control_helper(),
args),
num_idle_(num_subchannels()) {
// Need to maintain a ref to the LB policy as long as we maintain
// any references to subchannels, since the subchannels'
// pollset_sets will include the LB policy's pollset_set.
policy->Ref(DEBUG_LOCATION, "subchannel_list").release();
// Construct the ring.
RingHash::RingHashSubchannelList::Ring::Ring(
RingHashLbConfig* config, RingHashSubchannelList* subchannel_list,
const ChannelArgs& args) {
// Store the weights while finding the sum.
struct AddressWeight {
std::string address;
@ -481,9 +459,9 @@ RingHash::RingHashSubchannelList::RingHashSubchannelList(
};
std::vector<AddressWeight> address_weights;
size_t sum = 0;
address_weights.reserve(num_subchannels());
for (size_t i = 0; i < num_subchannels(); ++i) {
RingHashSubchannelData* sd = subchannel(i);
address_weights.reserve(subchannel_list->num_subchannels());
for (size_t i = 0; i < subchannel_list->num_subchannels(); ++i) {
RingHashSubchannelData* sd = subchannel_list->subchannel(i);
const ServerAddressWeightAttribute* weight_attribute = static_cast<
const ServerAddressWeightAttribute*>(sd->address().GetAttribute(
ServerAddressWeightAttribute::kServerAddressWeightAttributeKey));
@ -517,10 +495,8 @@ RingHash::RingHashSubchannelList::RingHashSubchannelList(
// to fit.
const size_t ring_size_cap = args.GetInt(GRPC_ARG_RING_HASH_LB_RING_SIZE_CAP)
.value_or(kRingSizeCapDefault);
const size_t min_ring_size =
std::min(policy->config_->min_ring_size(), ring_size_cap);
const size_t max_ring_size =
std::min(policy->config_->max_ring_size(), ring_size_cap);
const size_t min_ring_size = std::min(config->min_ring_size(), ring_size_cap);
const size_t max_ring_size = std::min(config->max_ring_size(), ring_size_cap);
const double scale = std::min(
std::ceil(min_normalized_weight * min_ring_size) / min_normalized_weight,
static_cast<double>(max_ring_size));
@ -537,7 +513,7 @@ RingHash::RingHashSubchannelList::RingHashSubchannelList(
double target_hashes = 0.0;
uint64_t min_hashes_per_host = ring_size;
uint64_t max_hashes_per_host = 0;
for (size_t i = 0; i < num_subchannels(); ++i) {
for (size_t i = 0; i < subchannel_list->num_subchannels(); ++i) {
const std::string& address_string = address_weights[i].address;
hash_key_buffer.assign(address_string.begin(), address_string.end());
hash_key_buffer.emplace_back('_');
@ -550,7 +526,7 @@ RingHash::RingHashSubchannelList::RingHashSubchannelList(
absl::string_view hash_key(hash_key_buffer.data(),
hash_key_buffer.size());
const uint64_t hash = XXH64(hash_key.data(), hash_key.size(), 0);
ring_.push_back({hash, subchannel(i)});
ring_.push_back({hash, i});
++count;
++current_hashes;
hash_key_buffer.erase(offset_start, hash_key_buffer.end());
@ -561,14 +537,34 @@ RingHash::RingHashSubchannelList::RingHashSubchannelList(
std::max(static_cast<uint64_t>(i), max_hashes_per_host);
}
std::sort(ring_.begin(), ring_.end(),
[](const RingHashSubchannelList::RingEntry& lhs,
const RingHashSubchannelList::RingEntry& rhs) -> bool {
[](const RingEntry& lhs, const RingEntry& rhs) -> bool {
return lhs.hash < rhs.hash;
});
}
//
// RingHash::RingHashSubchannelList
//
RingHash::RingHashSubchannelList::RingHashSubchannelList(
RingHash* policy, ServerAddressList addresses, const ChannelArgs& args)
: SubchannelList(policy,
(GRPC_TRACE_FLAG_ENABLED(grpc_lb_ring_hash_trace)
? "RingHashSubchannelList"
: nullptr),
std::move(addresses), policy->channel_control_helper(),
args),
num_idle_(num_subchannels()) {
// Need to maintain a ref to the LB policy as long as we maintain
// any references to subchannels, since the subchannels'
// pollset_sets will include the LB policy's pollset_set.
policy->Ref(DEBUG_LOCATION, "subchannel_list").release();
// Construct the ring.
ring_ = MakeRefCounted<Ring>(policy->config_.get(), this, args);
if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_ring_hash_trace)) {
gpr_log(GPR_INFO,
"[RH %p] created subchannel list %p with %" PRIuPTR " ring entries",
policy, this, ring_.size());
policy, this, ring_->ring().size());
}
}
@ -660,7 +656,7 @@ void RingHash::RingHashSubchannelList::UpdateRingHashConnectivityStateLocked(
// Note that we use our own picker regardless of connectivity state.
p->channel_control_helper()->UpdateState(
state, status,
MakeRefCounted<Picker>(Ref(DEBUG_LOCATION, "RingHashPicker")));
MakeRefCounted<Picker>(p->Ref(DEBUG_LOCATION, "RingHashPicker"), this));
// While the ring_hash policy is reporting TRANSIENT_FAILURE, it will
// not be getting any pick requests from the priority policy.
// However, because the ring_hash policy does not attempt to
@ -707,7 +703,6 @@ void RingHash::RingHashSubchannelData::ProcessConnectivityChangeLocked(
absl::optional<grpc_connectivity_state> old_state,
grpc_connectivity_state new_state) {
RingHash* p = static_cast<RingHash*>(subchannel_list()->policy());
grpc_connectivity_state last_connectivity_state = GetConnectivityState();
if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_ring_hash_trace)) {
gpr_log(
GPR_INFO,
@ -715,7 +710,7 @@ void RingHash::RingHashSubchannelData::ProcessConnectivityChangeLocked(
"(index %" PRIuPTR " of %" PRIuPTR "): prev_state=%s new_state=%s",
p, subchannel(), subchannel_list(), Index(),
subchannel_list()->num_subchannels(),
ConnectivityStateName(last_connectivity_state),
ConnectivityStateName(logical_connectivity_state_),
ConnectivityStateName(new_state));
}
GPR_ASSERT(subchannel() != nullptr);
@ -735,34 +730,23 @@ void RingHash::RingHashSubchannelData::ProcessConnectivityChangeLocked(
const bool connection_attempt_complete = new_state != GRPC_CHANNEL_CONNECTING;
// Decide what state to report for the purposes of aggregation and
// picker behavior.
// If the last recorded state was TRANSIENT_FAILURE, ignore the update
// unless the new state is READY.
bool update_status = true;
absl::Status status = connectivity_status();
if (last_connectivity_state == GRPC_CHANNEL_TRANSIENT_FAILURE &&
new_state != GRPC_CHANNEL_READY &&
new_state != GRPC_CHANNEL_TRANSIENT_FAILURE) {
new_state = GRPC_CHANNEL_TRANSIENT_FAILURE;
{
MutexLock lock(&mu_);
status = connectivity_status_;
}
update_status = false;
}
// Update state counters used for aggregation.
subchannel_list()->UpdateStateCountersLocked(last_connectivity_state,
new_state);
// Update status seen by picker if needed.
if (update_status) {
MutexLock lock(&mu_);
connectivity_status_ = connectivity_status();
// If the last recorded state was TRANSIENT_FAILURE, ignore the change
// unless the new state is READY (or TF again, in which case we need
// to update the status).
if (logical_connectivity_state_ != GRPC_CHANNEL_TRANSIENT_FAILURE ||
new_state == GRPC_CHANNEL_READY ||
new_state == GRPC_CHANNEL_TRANSIENT_FAILURE) {
// Update state counters used for aggregation.
subchannel_list()->UpdateStateCountersLocked(logical_connectivity_state_,
new_state);
// Update logical state.
logical_connectivity_state_ = new_state;
logical_connectivity_status_ = connectivity_status();
}
// Update last seen state, also used by picker.
connectivity_state_.store(new_state, std::memory_order_relaxed);
// Update the RH policy's connectivity state, creating new picker and new
// ring.
subchannel_list()->UpdateRingHashConnectivityStateLocked(
Index(), connection_attempt_complete, status);
Index(), connection_attempt_complete, logical_connectivity_status_);
}
//

@ -365,7 +365,6 @@ class RlsLb : public LoadBalancingPolicy {
class Picker : public LoadBalancingPolicy::SubchannelPicker {
public:
explicit Picker(RefCountedPtr<RlsLb> lb_policy);
~Picker() override;
PickResult Pick(PickArgs args) override;
@ -1008,19 +1007,6 @@ RlsLb::Picker::Picker(RefCountedPtr<RlsLb> lb_policy)
}
}
RlsLb::Picker::~Picker() {
// It's not safe to unref the default child policy in the picker,
// since that needs to be done in the WorkSerializer.
if (default_child_policy_ != nullptr) {
auto* default_child_policy = default_child_policy_.release();
lb_policy_->work_serializer()->Run(
[default_child_policy]() {
default_child_policy->Unref(DEBUG_LOCATION, "Picker");
},
DEBUG_LOCATION);
}
}
LoadBalancingPolicy::PickResult RlsLb::Picker::Pick(PickArgs args) {
// Construct key for request.
RequestKey key = {BuildKeyMap(config_->key_builder_map(), args.path,

@ -18,13 +18,16 @@
#include <inttypes.h>
#include <stdlib.h>
#include <string.h>
#include <algorithm>
#include <atomic>
#include <memory>
#include <string>
#include <utility>
#include <vector>
#include "absl/random/random.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
#include "absl/strings/str_cat.h"
@ -174,7 +177,7 @@ class RoundRobin : public LoadBalancingPolicy {
// Using pointer value only, no ref held -- do not dereference!
RoundRobin* parent_;
size_t last_picked_index_;
std::atomic<size_t> last_picked_index_;
std::vector<RefCountedPtr<SubchannelInterface>> subchannels_;
};
@ -189,6 +192,8 @@ class RoundRobin : public LoadBalancingPolicy {
RefCountedPtr<RoundRobinSubchannelList> latest_pending_subchannel_list_;
bool shutdown_ = false;
absl::BitGen bit_gen_;
};
//
@ -207,27 +212,26 @@ RoundRobin::Picker::Picker(RoundRobin* parent,
}
// For discussion on why we generate a random starting index for
// the picker, see https://github.com/grpc/grpc-go/issues/2580.
// TODO(roth): rand(3) is not thread-safe. This should be replaced with
// something better as part of https://github.com/grpc/grpc/issues/17891.
last_picked_index_ = rand() % subchannels_.size();
size_t index =
absl::Uniform<size_t>(parent->bit_gen_, 0, subchannels_.size());
last_picked_index_.store(index, std::memory_order_relaxed);
if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_round_robin_trace)) {
gpr_log(GPR_INFO,
"[RR %p picker %p] created picker from subchannel_list=%p "
"with %" PRIuPTR " READY subchannels; last_picked_index_=%" PRIuPTR,
parent_, this, subchannel_list, subchannels_.size(),
last_picked_index_);
parent_, this, subchannel_list, subchannels_.size(), index);
}
}
RoundRobin::PickResult RoundRobin::Picker::Pick(PickArgs /*args*/) {
last_picked_index_ = (last_picked_index_ + 1) % subchannels_.size();
size_t index = last_picked_index_.fetch_add(1, std::memory_order_relaxed) %
subchannels_.size();
if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_round_robin_trace)) {
gpr_log(GPR_INFO,
"[RR %p picker %p] returning index %" PRIuPTR ", subchannel=%p",
parent_, this, last_picked_index_,
subchannels_[last_picked_index_].get());
parent_, this, index, subchannels_[index].get());
}
return PickResult::Complete(subchannels_[last_picked_index_]);
return PickResult::Complete(subchannels_[index]);
}
//

@ -26,6 +26,7 @@
#include <utility>
#include <vector>
#include "absl/base/thread_annotations.h"
#include "absl/random/random.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
@ -46,6 +47,7 @@
#include "src/core/lib/gprpp/debug_location.h"
#include "src/core/lib/gprpp/orphanable.h"
#include "src/core/lib/gprpp/ref_counted_ptr.h"
#include "src/core/lib/gprpp/sync.h"
#include "src/core/lib/gprpp/time.h"
#include "src/core/lib/gprpp/validation_errors.h"
#include "src/core/lib/gprpp/work_serializer.h"
@ -138,7 +140,11 @@ class WeightedTargetLb : public LoadBalancingPolicy {
private:
PickerList pickers_;
absl::BitGen bit_gen_;
// TODO(roth): Consider using a separate thread-local BitGen for each CPU
// to avoid the need for this mutex.
Mutex mu_;
absl::BitGen bit_gen_ ABSL_GUARDED_BY(&mu_);
};
// Each WeightedChild holds a ref to its parent WeightedTargetLb.
@ -247,8 +253,10 @@ class WeightedTargetLb : public LoadBalancingPolicy {
WeightedTargetLb::PickResult WeightedTargetLb::WeightedPicker::Pick(
PickArgs args) {
// Generate a random number in [0, total weight).
const uint64_t key =
absl::Uniform<uint64_t>(bit_gen_, 0, pickers_.back().first);
const uint64_t key = [&]() {
MutexLock lock(&mu_);
return absl::Uniform<uint64_t>(bit_gen_, 0, pickers_.back().first);
}();
// Find the index in pickers_ corresponding to key.
size_t mid = 0;
size_t start_index = 0;

@ -1351,8 +1351,9 @@ RetryFilter::CallData::CallAttempt::BatchData::~BatchData() {
this);
}
CallAttempt* call_attempt = std::exchange(call_attempt_, nullptr);
GRPC_CALL_STACK_UNREF(call_attempt->calld_->owning_call_, "Retry BatchData");
grpc_call_stack* owning_call = call_attempt->calld_->owning_call_;
call_attempt->Unref(DEBUG_LOCATION, "~BatchData");
GRPC_CALL_STACK_UNREF(owning_call, "Retry BatchData");
}
void RetryFilter::CallData::CallAttempt::BatchData::

@ -93,11 +93,14 @@ std::string XdsEndpointResource::Priority::ToString() const {
}
bool XdsEndpointResource::DropConfig::ShouldDrop(
const std::string** category_name) const {
const std::string** category_name) {
for (size_t i = 0; i < drop_category_list_.size(); ++i) {
const auto& drop_category = drop_category_list_[i];
// Generate a random number in [0, 1000000).
const uint32_t random = static_cast<uint32_t>(rand()) % 1000000;
const uint32_t random = [&]() {
MutexLock lock(&mu_);
return absl::Uniform<uint32_t>(bit_gen_, 0, 1000000);
}();
if (random < drop_category.parts_per_million) {
*category_name = &drop_category.name;
return true;

@ -28,6 +28,8 @@
#include <utility>
#include <vector>
#include "absl/base/thread_annotations.h"
#include "absl/random/random.h"
#include "absl/strings/string_view.h"
#include "envoy/config/endpoint/v3/endpoint.upbdefs.h"
#include "upb/def.h"
@ -38,6 +40,7 @@
#include "src/core/ext/xds/xds_resource_type_impl.h"
#include "src/core/lib/gprpp/ref_counted.h"
#include "src/core/lib/gprpp/ref_counted_ptr.h"
#include "src/core/lib/gprpp/sync.h"
#include "src/core/lib/resolver/server_address.h"
namespace grpc_core {
@ -90,7 +93,7 @@ struct XdsEndpointResource : public XdsResourceType::ResourceData {
// The only method invoked from outside the WorkSerializer (used in
// the data plane).
bool ShouldDrop(const std::string** category_name) const;
bool ShouldDrop(const std::string** category_name);
const DropCategoryList& drop_category_list() const {
return drop_category_list_;
@ -108,6 +111,11 @@ struct XdsEndpointResource : public XdsResourceType::ResourceData {
private:
DropCategoryList drop_category_list_;
bool drop_all_ = false;
// TODO(roth): Consider using a separate thread-local BitGen for each CPU
// to avoid the need for this mutex.
Mutex mu_;
absl::BitGen bit_gen_ ABSL_GUARDED_BY(&mu_);
};
PriorityList priorities;

@ -69,19 +69,15 @@ LoadBalancingPolicy::SubchannelPicker::SubchannelPicker()
LoadBalancingPolicy::PickResult LoadBalancingPolicy::QueuePicker::Pick(
PickArgs /*args*/) {
// We invoke the parent's ExitIdleLocked() via a closure instead
// of doing it directly here, for two reasons:
// 1. ExitIdleLocked() may cause the policy's state to change and
// a new picker to be delivered to the channel. If that new
// picker is delivered before ExitIdleLocked() returns, then by
// the time this function returns, the pick will already have
// been processed, and we'll be trying to re-process the same
// pick again, leading to a crash.
// 2. We are currently running in the data plane mutex, but we
// need to bounce into the control plane work_serializer to call
// ExitIdleLocked().
if (!exit_idle_called_ && parent_ != nullptr) {
exit_idle_called_ = true;
auto* parent = parent_->Ref().release(); // ref held by lambda.
// of doing it directly here because ExitIdleLocked() may cause the
// policy's state to change and a new picker to be delivered to the
// channel. If that new picker is delivered before ExitIdleLocked()
// returns, then by the time this function returns, the pick will already
// have been processed, and we'll be trying to re-process the same pick
// again, leading to a crash.
MutexLock lock(&mu_);
if (parent_ != nullptr) {
auto* parent = parent_.release(); // ref held by lambda.
ExecCtx::Run(DEBUG_LOCATION,
GRPC_CLOSURE_CREATE(
[](void* arg, grpc_error_handle /*error*/) {

@ -27,6 +27,7 @@
#include <utility>
#include <vector>
#include "absl/base/thread_annotations.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
#include "absl/strings/string_view.h"
@ -44,6 +45,7 @@
#include "src/core/lib/gprpp/orphanable.h"
#include "src/core/lib/gprpp/ref_counted.h"
#include "src/core/lib/gprpp/ref_counted_ptr.h"
#include "src/core/lib/gprpp/sync.h"
#include "src/core/lib/gprpp/work_serializer.h"
#include "src/core/lib/iomgr/iomgr_fwd.h"
#include "src/core/lib/load_balancing/subchannel_interface.h"
@ -392,8 +394,8 @@ class LoadBalancingPolicy : public InternallyRefCounted<LoadBalancingPolicy> {
PickResult Pick(PickArgs args) override;
private:
RefCountedPtr<LoadBalancingPolicy> parent_;
bool exit_idle_called_ = false;
Mutex mu_;
RefCountedPtr<LoadBalancingPolicy> parent_ ABSL_GUARDED_BY(&mu_);
};
// A picker that returns PickResult::Fail for all picks.

Loading…
Cancel
Save