Skip to content

Commit b549e15

Browse files
authored
Revert "Add std::shared_ptr support to ChannelArgs, and precondition ChannelArgs with a default EventEngine (grpc#30128)" (grpc#30170)
This reverts commit e28b70a.
1 parent 80541db commit b549e15

File tree

10 files changed

+13
-242
lines changed

10 files changed

+13
-242
lines changed

BUILD

-7
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,6 @@ grpc_cc_library(
419419
"channel_stack_type",
420420
"config",
421421
"default_event_engine_factory_hdrs",
422-
"event_engine_base",
423422
"gpr_base",
424423
"grpc_authorization_base",
425424
"grpc_base",
@@ -479,7 +478,6 @@ grpc_cc_library(
479478
"channel_stack_type",
480479
"config",
481480
"default_event_engine_factory_hdrs",
482-
"event_engine_base",
483481
"gpr_base",
484482
"grpc_authorization_base",
485483
"grpc_base",
@@ -2255,7 +2253,6 @@ grpc_cc_library(
22552253
"src/core/lib/event_engine/event_engine_factory.h",
22562254
],
22572255
deps = [
2258-
"config",
22592256
"event_engine_base_hdrs",
22602257
"gpr_base",
22612258
],
@@ -2412,9 +2409,6 @@ grpc_cc_library(
24122409
"src/core/lib/event_engine/event_engine.cc",
24132410
],
24142411
deps = [
2415-
"channel_args",
2416-
"channel_args_preconditioning",
2417-
"config",
24182412
"default_event_engine_factory",
24192413
"default_event_engine_factory_hdrs",
24202414
"event_engine_base_hdrs",
@@ -3076,7 +3070,6 @@ grpc_cc_library(
30763070
"avl",
30773071
"channel_stack_type",
30783072
"dual_ref_counted",
3079-
"event_engine_base_hdrs",
30803073
"gpr_base",
30813074
"gpr_platform",
30823075
"grpc_codegen",

include/grpc/event_engine/event_engine.h

-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
#include <grpc/event_engine/memory_allocator.h>
2727
#include <grpc/event_engine/port.h>
2828
#include <grpc/event_engine/slice_buffer.h>
29-
#include <grpc/impl/codegen/grpc_types.h>
3029

3130
// TODO(vigneshbabu): Define the Endpoint::Write metrics collection system
3231
namespace grpc_event_engine {

include/grpc/impl/codegen/grpc_types.h

-2
Original file line numberDiff line numberDiff line change
@@ -455,8 +455,6 @@ typedef struct {
455455
* channel arg. Int valued, milliseconds. Defaults to 10 minutes.*/
456456
#define GRPC_ARG_SERVER_CONFIG_CHANGE_DRAIN_GRACE_TIME_MS \
457457
"grpc.experimental.server_config_change_drain_grace_time_ms"
458-
/** EventEngine pointer */
459-
#define GRPC_ARG_EVENT_ENGINE "grpc.event_engine"
460458
/** \} */
461459

462460
/** Result of a grpc call. If the caller satisfies the prerequisites of a

src/core/ext/filters/client_channel/subchannel.cc

+4-6
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
#include "src/core/lib/config/core_configuration.h"
5151
#include "src/core/lib/debug/stats.h"
5252
#include "src/core/lib/debug/trace.h"
53+
#include "src/core/lib/event_engine/event_engine_factory.h"
5354
#include "src/core/lib/gpr/alloc.h"
5455
#include "src/core/lib/gprpp/debug_location.h"
5556
#include "src/core/lib/gprpp/ref_counted_ptr.h"
@@ -82,8 +83,7 @@
8283
GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(SubchannelCall)))
8384

8485
namespace grpc_core {
85-
86-
using ::grpc_event_engine::experimental::EventEngine;
86+
using ::grpc_event_engine::experimental::GetDefaultEventEngine;
8787

8888
TraceFlag grpc_trace_subchannel(false, "subchannel");
8989
DebugOnlyTraceFlag grpc_trace_subchannel_refcount(false, "subchannel_refcount");
@@ -675,8 +675,6 @@ Subchannel::Subchannel(SubchannelKey key,
675675
} else {
676676
args_ = grpc_channel_args_copy(args);
677677
}
678-
// Initialize EventEngine
679-
event_engine_ = ChannelArgs::FromC(args_).GetObjectRef<EventEngine>();
680678
// Initialize channelz.
681679
const bool channelz_enabled = grpc_channel_args_find_bool(
682680
args_, GRPC_ARG_ENABLE_CHANNELZ, GRPC_ENABLE_CHANNELZ_DEFAULT);
@@ -804,7 +802,7 @@ void Subchannel::ResetBackoff() {
804802
MutexLock lock(&mu_);
805803
backoff_.Reset();
806804
if (state_ == GRPC_CHANNEL_TRANSIENT_FAILURE &&
807-
event_engine_->Cancel(retry_timer_handle_)) {
805+
GetDefaultEventEngine()->Cancel(retry_timer_handle_)) {
808806
OnRetryTimerLocked();
809807
} else if (state_ == GRPC_CHANNEL_CONNECTING) {
810808
next_attempt_time_ = ExecCtx::Get()->Now();
@@ -950,7 +948,7 @@ void Subchannel::OnConnectingFinishedLocked(grpc_error_handle error) {
950948
time_until_next_attempt.millis());
951949
SetConnectivityStateLocked(GRPC_CHANNEL_TRANSIENT_FAILURE,
952950
grpc_error_to_absl_status(error));
953-
retry_timer_handle_ = event_engine_->RunAfter(
951+
retry_timer_handle_ = GetDefaultEventEngine()->RunAfter(
954952
time_until_next_attempt,
955953
[self = WeakRef(DEBUG_LOCATION, "RetryTimer")]() mutable {
956954
{

src/core/ext/filters/client_channel/subchannel.h

-5
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323

2424
#include <deque>
2525
#include <map>
26-
#include <memory>
2726
#include <string>
2827

2928
#include "absl/base/thread_annotations.h"
@@ -420,10 +419,6 @@ class Subchannel : public DualRefCounted<Subchannel> {
420419
// Data producer map.
421420
std::map<UniqueTypeName, DataProducerInterface*> data_producer_map_
422421
ABSL_GUARDED_BY(mu_);
423-
424-
// Engine used in this Subchannel
425-
std::shared_ptr<grpc_event_engine::experimental::EventEngine> event_engine_
426-
ABSL_GUARDED_BY(mu_);
427422
};
428423

429424
} // namespace grpc_core

src/core/lib/channel/channel_args.h

+9-110
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
#include <stddef.h>
2525

2626
#include <algorithm> // IWYU pragma: keep
27-
#include <memory>
2827
#include <string>
2928
#include <type_traits>
3029
#include <utility>
@@ -34,7 +33,6 @@
3433
#include "absl/types/optional.h"
3534
#include "absl/types/variant.h"
3635

37-
#include <grpc/event_engine/event_engine.h>
3836
#include <grpc/impl/codegen/grpc_types.h>
3937

4038
#include "src/core/lib/avl/avl.h"
@@ -54,7 +52,7 @@ namespace grpc_core {
5452
// ChannelArgs to automatically derive a vtable from a T*.
5553
// To participate as a pointer, instances should expose the function:
5654
// // Gets the vtable for this type
57-
// static const grpc_arg_pointer_vtable* VTable();
55+
// static const grpc_channel_arg_vtable* VTable();
5856
// // Performs any mutations required for channel args to own a pointer
5957
// // Only needed if ChannelArgs::Set is to be called with a raw pointer.
6058
// static void* TakeUnownedPointer(T* p);
@@ -88,32 +86,6 @@ struct ChannelArgTypeTraits<
8886
};
8987
};
9088

91-
// Specialization for shared_ptr
92-
// Incurs an allocation because shared_ptr.release is not a thing.
93-
template <typename T>
94-
struct is_shared_ptr : std::false_type {};
95-
template <typename T>
96-
struct is_shared_ptr<std::shared_ptr<T>> : std::true_type {};
97-
template <typename T>
98-
struct ChannelArgTypeTraits<T,
99-
absl::enable_if_t<is_shared_ptr<T>::value, void>> {
100-
static void* TakeUnownedPointer(T* p) { return p; }
101-
static const grpc_arg_pointer_vtable* VTable() {
102-
static const grpc_arg_pointer_vtable tbl = {
103-
// copy
104-
[](void* p) -> void* { return new T(*static_cast<T*>(p)); },
105-
// destroy
106-
[](void* p) { delete static_cast<T*>(p); },
107-
// compare
108-
[](void* p1, void* p2) {
109-
return QsortCompare(static_cast<const T*>(p1)->get(),
110-
static_cast<const T*>(p2)->get());
111-
},
112-
};
113-
return &tbl;
114-
};
115-
};
116-
11789
// If a type declares some member 'struct RawPointerChannelArgTag {}' then
11890
// we automatically generate a vtable for it that does not do any ownership
11991
// management and compares the type by pointer identity.
@@ -136,54 +108,6 @@ struct ChannelArgTypeTraits<T,
136108
};
137109
};
138110

139-
// GetObject support for shared_ptr and RefCountedPtr
140-
template <typename T>
141-
struct WrapInSharedPtr
142-
: std::integral_constant<
143-
bool, std::is_base_of<std::enable_shared_from_this<T>, T>::value> {};
144-
template <>
145-
struct WrapInSharedPtr<grpc_event_engine::experimental::EventEngine>
146-
: std::true_type {};
147-
template <typename T, typename Ignored = void /* for SFINAE */>
148-
struct GetObjectImpl;
149-
// std::shared_ptr implementation
150-
template <typename T>
151-
struct GetObjectImpl<T, absl::enable_if_t<WrapInSharedPtr<T>::value, void>> {
152-
using Result = T*;
153-
using ReffedResult = std::shared_ptr<T>;
154-
using StoredType = std::shared_ptr<T>*;
155-
static Result Get(StoredType p) { return p->get(); };
156-
static ReffedResult GetReffed(StoredType p) { return ReffedResult(*p); };
157-
};
158-
// RefCountedPtr
159-
template <typename T>
160-
struct GetObjectImpl<T, absl::enable_if_t<!WrapInSharedPtr<T>::value, void>> {
161-
using Result = T*;
162-
using ReffedResult = RefCountedPtr<T>;
163-
using StoredType = Result;
164-
static Result Get(StoredType p) { return p; };
165-
static ReffedResult GetReffed(StoredType p) {
166-
if (p == nullptr) return nullptr;
167-
return p->Ref();
168-
};
169-
};
170-
171-
// Provide the canonical name for a type's channel arg key
172-
template <typename T>
173-
struct ChannelArgNameTraits {
174-
static absl::string_view ChannelArgName() { return T::ChannelArgName(); }
175-
};
176-
template <typename T>
177-
struct ChannelArgNameTraits<std::shared_ptr<T>> {
178-
static absl::string_view ChannelArgName() { return T::ChannelArgName(); }
179-
};
180-
181-
// Specialization for the EventEngine
182-
template <>
183-
struct ChannelArgNameTraits<grpc_event_engine::experimental::EventEngine> {
184-
static absl::string_view ChannelArgName() { return GRPC_ARG_EVENT_ENGINE; }
185-
};
186-
187111
class ChannelArgs {
188112
public:
189113
class Pointer {
@@ -278,40 +202,19 @@ class ChannelArgs {
278202
absl::remove_cvref_t<decltype(*store_value)>>::VTable()));
279203
}
280204
template <typename T>
281-
GRPC_MUST_USE_RESULT absl::enable_if_t<
282-
std::is_same<
283-
const grpc_arg_pointer_vtable*,
284-
decltype(ChannelArgTypeTraits<std::shared_ptr<T>>::VTable())>::value,
285-
ChannelArgs>
286-
Set(absl::string_view name, std::shared_ptr<T> value) const {
287-
auto* store_value = new std::shared_ptr<T>(value);
288-
return Set(
289-
name,
290-
Pointer(ChannelArgTypeTraits<std::shared_ptr<T>>::TakeUnownedPointer(
291-
store_value),
292-
ChannelArgTypeTraits<std::shared_ptr<T>>::VTable()));
293-
}
294-
template <typename T>
295205
GRPC_MUST_USE_RESULT ChannelArgs SetIfUnset(absl::string_view name, T value) {
296206
if (Contains(name)) return *this;
297207
return Set(name, std::move(value));
298208
}
299209
GRPC_MUST_USE_RESULT ChannelArgs Remove(absl::string_view name) const;
300210
bool Contains(absl::string_view name) const { return Get(name) != nullptr; }
301211

302-
template <typename T>
303-
bool ContainsObject() const {
304-
return Get(ChannelArgNameTraits<T>::ChannelArgName()) != nullptr;
305-
}
306-
307212
absl::optional<int> GetInt(absl::string_view name) const;
308213
absl::optional<absl::string_view> GetString(absl::string_view name) const;
309214
void* GetVoidPointer(absl::string_view name) const;
310215
template <typename T>
311-
typename GetObjectImpl<T>::StoredType GetPointer(
312-
absl::string_view name) const {
313-
return static_cast<typename GetObjectImpl<T>::StoredType>(
314-
GetVoidPointer(name));
216+
T* GetPointer(absl::string_view name) const {
217+
return static_cast<T*>(GetVoidPointer(name));
315218
}
316219
absl::optional<Duration> GetDurationFromIntMillis(
317220
absl::string_view name) const;
@@ -331,18 +234,14 @@ class ChannelArgs {
331234
return Set(T::ChannelArgName(), std::move(p));
332235
}
333236
template <typename T>
334-
GRPC_MUST_USE_RESULT ChannelArgs SetObject(std::shared_ptr<T> p) const {
335-
return Set(ChannelArgNameTraits<T>::ChannelArgName(), std::move(p));
336-
}
337-
template <typename T>
338-
typename GetObjectImpl<T>::Result GetObject() {
339-
return GetObjectImpl<T>::Get(
340-
GetPointer<T>(ChannelArgNameTraits<T>::ChannelArgName()));
237+
T* GetObject() {
238+
return GetPointer<T>(T::ChannelArgName());
341239
}
342240
template <typename T>
343-
typename GetObjectImpl<T>::ReffedResult GetObjectRef() {
344-
return GetObjectImpl<T>::GetReffed(
345-
GetPointer<T>(ChannelArgNameTraits<T>::ChannelArgName()));
241+
RefCountedPtr<T> GetObjectRef() {
242+
auto* p = GetObject<T>();
243+
if (p == nullptr) return nullptr;
244+
return p->Ref();
346245
}
347246

348247
bool operator<(const ChannelArgs& other) const { return args_ < other.args_; }

src/core/lib/event_engine/event_engine.cc

-25
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,6 @@
2020

2121
#include <grpc/event_engine/event_engine.h>
2222

23-
#include "src/core/lib/channel/channel_args.h"
24-
#include "src/core/lib/channel/channel_args_preconditioning.h"
25-
#include "src/core/lib/config/core_configuration.h"
2623
#include "src/core/lib/event_engine/event_engine_factory.h"
2724

2825
namespace grpc_event_engine {
@@ -66,27 +63,5 @@ void ResetDefaultEventEngine() {
6663
delete g_event_engine.exchange(nullptr, std::memory_order_acq_rel);
6764
}
6865

69-
namespace {
70-
grpc_core::ChannelArgs EnsureEventEngineInChannelArgs(
71-
grpc_core::ChannelArgs args) {
72-
if (args.ContainsObject<EventEngine>()) return args;
73-
// TODO(hork): Consider deleting GetDefaultEventEngine(), use the factory
74-
// directly when ChannelArgs aren't available. That would eliminate the no-op
75-
// deleter below.
76-
// Store a shared_ptr with a no-op deleter. The default is expected to live
77-
// indefinitely.
78-
return args.SetObject<EventEngine>(
79-
std::shared_ptr<EventEngine>(GetDefaultEventEngine(), [](auto) {}));
80-
}
81-
} // namespace
82-
8366
} // namespace experimental
8467
} // namespace grpc_event_engine
85-
86-
namespace grpc_core {
87-
void RegisterEventEngine(CoreConfiguration::Builder* builder) {
88-
builder->channel_args_preconditioning()->RegisterStage(
89-
grpc_event_engine::experimental::EnsureEventEngineInChannelArgs);
90-
}
91-
92-
} // namespace grpc_core

src/core/lib/event_engine/event_engine_factory.h

-6
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,6 @@
2020

2121
#include <grpc/event_engine/event_engine.h>
2222

23-
#include "src/core/lib/config/core_configuration.h"
24-
2523
namespace grpc_event_engine {
2624
namespace experimental {
2725

@@ -37,10 +35,6 @@ std::unique_ptr<EventEngine> DefaultEventEngineFactory();
3735
/// Reset the default event engine
3836
void ResetDefaultEventEngine();
3937

40-
/// On ingress, ensure that an EventEngine exists in channel args via
41-
/// preconditioning.
42-
void RegisterEventEngine(grpc_core::CoreConfiguration::Builder* builder);
43-
4438
} // namespace experimental
4539
} // namespace grpc_event_engine
4640

src/core/plugin_registry/grpc_plugin_registry.cc

-2
Original file line numberDiff line numberDiff line change
@@ -105,10 +105,8 @@ extern void RegisterFakeResolver(CoreConfiguration::Builder* builder);
105105
#ifdef GPR_SUPPORT_BINDER_TRANSPORT
106106
extern void RegisterBinderResolver(CoreConfiguration::Builder* builder);
107107
#endif
108-
extern void RegisterEventEngine(CoreConfiguration::Builder* builder);
109108

110109
void BuildCoreConfiguration(CoreConfiguration::Builder* builder) {
111-
RegisterEventEngine(builder);
112110
// The order of the handshaker registration is crucial here.
113111
// We want TCP connect handshaker to be registered last so that it is added to
114112
// the start of the handshaker list.

0 commit comments

Comments
 (0)