From d14020d4e272770cd38af4ecc59e2b9e2161cd8f Mon Sep 17 00:00:00 2001 From: AJ Heller Date: Wed, 25 Jan 2023 17:31:34 -0800 Subject: [PATCH] [EventEngine] Add invalid handle types to the public API (#32202) * [EventEngine] Add invalid handle types to the public API * Automated change: Fix sanity tests * add definition for static constexpr members * sanitize Co-authored-by: drfloob --- CMakeLists.txt | 5 ++++ Makefile | 2 ++ build_autogenerated.yaml | 5 ++++ config.m4 | 1 + config.w32 | 1 + gRPC-Core.podspec | 1 + grpc.gemspec | 1 + grpc.gyp | 3 +++ include/grpc/event_engine/event_engine.h | 3 +++ package.xml | 1 + src/core/BUILD | 1 + src/core/lib/event_engine/event_engine.cc | 25 +++++++++++++++++++ .../event_engine/posix_engine/posix_engine.cc | 8 +++--- .../event_engine/windows/windows_engine.cc | 16 ++++++------ .../lib/event_engine/windows/windows_engine.h | 4 --- src/python/grpcio/grpc_core_dependencies.py | 1 + tools/doxygen/Doxyfile.c++.internal | 1 + tools/doxygen/Doxyfile.core.internal | 1 + 18 files changed, 64 insertions(+), 16 deletions(-) create mode 100644 src/core/lib/event_engine/event_engine.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index b3107cce4df..4dd7544396a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2167,6 +2167,7 @@ add_library(grpc src/core/lib/event_engine/channel_args_endpoint_config.cc src/core/lib/event_engine/default_event_engine.cc src/core/lib/event_engine/default_event_engine_factory.cc + src/core/lib/event_engine/event_engine.cc src/core/lib/event_engine/forkable.cc src/core/lib/event_engine/memory_allocator.cc src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -2844,6 +2845,7 @@ add_library(grpc_unsecure src/core/lib/event_engine/channel_args_endpoint_config.cc src/core/lib/event_engine/default_event_engine.cc src/core/lib/event_engine/default_event_engine_factory.cc + src/core/lib/event_engine/event_engine.cc src/core/lib/event_engine/forkable.cc src/core/lib/event_engine/memory_allocator.cc src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -4336,6 +4338,7 @@ add_library(grpc_authorization_provider src/core/lib/event_engine/channel_args_endpoint_config.cc src/core/lib/event_engine/default_event_engine.cc src/core/lib/event_engine/default_event_engine_factory.cc + src/core/lib/event_engine/event_engine.cc src/core/lib/event_engine/forkable.cc src/core/lib/event_engine/memory_allocator.cc src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -11403,6 +11406,7 @@ add_executable(frame_test src/core/lib/event_engine/channel_args_endpoint_config.cc src/core/lib/event_engine/default_event_engine.cc src/core/lib/event_engine/default_event_engine_factory.cc + src/core/lib/event_engine/event_engine.cc src/core/lib/event_engine/forkable.cc src/core/lib/event_engine/memory_allocator.cc src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -20131,6 +20135,7 @@ endif() if(gRPC_BUILD_TESTS) add_executable(test_core_event_engine_slice_buffer_test + src/core/lib/event_engine/event_engine.cc src/core/lib/event_engine/resolved_address.cc src/core/lib/event_engine/slice.cc src/core/lib/event_engine/slice_buffer.cc diff --git a/Makefile b/Makefile index 2687a0414fd..f33b24dc103 100644 --- a/Makefile +++ b/Makefile @@ -1424,6 +1424,7 @@ LIBGRPC_SRC = \ src/core/lib/event_engine/channel_args_endpoint_config.cc \ src/core/lib/event_engine/default_event_engine.cc \ src/core/lib/event_engine/default_event_engine_factory.cc \ + src/core/lib/event_engine/event_engine.cc \ src/core/lib/event_engine/forkable.cc \ src/core/lib/event_engine/memory_allocator.cc \ src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc \ @@ -1960,6 +1961,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/event_engine/channel_args_endpoint_config.cc \ src/core/lib/event_engine/default_event_engine.cc \ src/core/lib/event_engine/default_event_engine_factory.cc \ + src/core/lib/event_engine/event_engine.cc \ src/core/lib/event_engine/forkable.cc \ src/core/lib/event_engine/memory_allocator.cc \ src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc \ diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 7e40b8af917..d656f1b5ba4 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -1559,6 +1559,7 @@ libs: - src/core/lib/event_engine/channel_args_endpoint_config.cc - src/core/lib/event_engine/default_event_engine.cc - src/core/lib/event_engine/default_event_engine_factory.cc + - src/core/lib/event_engine/event_engine.cc - src/core/lib/event_engine/forkable.cc - src/core/lib/event_engine/memory_allocator.cc - src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -2503,6 +2504,7 @@ libs: - src/core/lib/event_engine/channel_args_endpoint_config.cc - src/core/lib/event_engine/default_event_engine.cc - src/core/lib/event_engine/default_event_engine_factory.cc + - src/core/lib/event_engine/event_engine.cc - src/core/lib/event_engine/forkable.cc - src/core/lib/event_engine/memory_allocator.cc - src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -3831,6 +3833,7 @@ libs: - src/core/lib/event_engine/channel_args_endpoint_config.cc - src/core/lib/event_engine/default_event_engine.cc - src/core/lib/event_engine/default_event_engine_factory.cc + - src/core/lib/event_engine/event_engine.cc - src/core/lib/event_engine/forkable.cc - src/core/lib/event_engine/memory_allocator.cc - src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -7610,6 +7613,7 @@ targets: - src/core/lib/event_engine/channel_args_endpoint_config.cc - src/core/lib/event_engine/default_event_engine.cc - src/core/lib/event_engine/default_event_engine_factory.cc + - src/core/lib/event_engine/event_engine.cc - src/core/lib/event_engine/forkable.cc - src/core/lib/event_engine/memory_allocator.cc - src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -11567,6 +11571,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_string_helpers.h src: + - src/core/lib/event_engine/event_engine.cc - src/core/lib/event_engine/resolved_address.cc - src/core/lib/event_engine/slice.cc - src/core/lib/event_engine/slice_buffer.cc diff --git a/config.m4 b/config.m4 index c4a1b6a5d5d..0b98a18c3fa 100644 --- a/config.m4 +++ b/config.m4 @@ -506,6 +506,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/event_engine/channel_args_endpoint_config.cc \ src/core/lib/event_engine/default_event_engine.cc \ src/core/lib/event_engine/default_event_engine_factory.cc \ + src/core/lib/event_engine/event_engine.cc \ src/core/lib/event_engine/forkable.cc \ src/core/lib/event_engine/memory_allocator.cc \ src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc \ diff --git a/config.w32 b/config.w32 index b0c69178baa..7c0203053fa 100644 --- a/config.w32 +++ b/config.w32 @@ -472,6 +472,7 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\event_engine\\channel_args_endpoint_config.cc " + "src\\core\\lib\\event_engine\\default_event_engine.cc " + "src\\core\\lib\\event_engine\\default_event_engine_factory.cc " + + "src\\core\\lib\\event_engine\\event_engine.cc " + "src\\core\\lib\\event_engine\\forkable.cc " + "src\\core\\lib\\event_engine\\memory_allocator.cc " + "src\\core\\lib\\event_engine\\posix_engine\\ev_epoll1_linux.cc " + diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 2e202149607..d12006b0986 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -1128,6 +1128,7 @@ Pod::Spec.new do |s| 'src/core/lib/event_engine/default_event_engine.h', 'src/core/lib/event_engine/default_event_engine_factory.cc', 'src/core/lib/event_engine/default_event_engine_factory.h', + 'src/core/lib/event_engine/event_engine.cc', 'src/core/lib/event_engine/executor/executor.h', 'src/core/lib/event_engine/forkable.cc', 'src/core/lib/event_engine/forkable.h', diff --git a/grpc.gemspec b/grpc.gemspec index d4e49dcc928..f92e3714674 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1039,6 +1039,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/event_engine/default_event_engine.h ) s.files += %w( src/core/lib/event_engine/default_event_engine_factory.cc ) s.files += %w( src/core/lib/event_engine/default_event_engine_factory.h ) + s.files += %w( src/core/lib/event_engine/event_engine.cc ) s.files += %w( src/core/lib/event_engine/executor/executor.h ) s.files += %w( src/core/lib/event_engine/forkable.cc ) s.files += %w( src/core/lib/event_engine/forkable.h ) diff --git a/grpc.gyp b/grpc.gyp index 910cbce0e3a..aac09f741a8 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -837,6 +837,7 @@ 'src/core/lib/event_engine/channel_args_endpoint_config.cc', 'src/core/lib/event_engine/default_event_engine.cc', 'src/core/lib/event_engine/default_event_engine_factory.cc', + 'src/core/lib/event_engine/event_engine.cc', 'src/core/lib/event_engine/forkable.cc', 'src/core/lib/event_engine/memory_allocator.cc', 'src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc', @@ -1315,6 +1316,7 @@ 'src/core/lib/event_engine/channel_args_endpoint_config.cc', 'src/core/lib/event_engine/default_event_engine.cc', 'src/core/lib/event_engine/default_event_engine_factory.cc', + 'src/core/lib/event_engine/event_engine.cc', 'src/core/lib/event_engine/forkable.cc', 'src/core/lib/event_engine/memory_allocator.cc', 'src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc', @@ -1818,6 +1820,7 @@ 'src/core/lib/event_engine/channel_args_endpoint_config.cc', 'src/core/lib/event_engine/default_event_engine.cc', 'src/core/lib/event_engine/default_event_engine_factory.cc', + 'src/core/lib/event_engine/event_engine.cc', 'src/core/lib/event_engine/forkable.cc', 'src/core/lib/event_engine/memory_allocator.cc', 'src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc', diff --git a/include/grpc/event_engine/event_engine.h b/include/grpc/event_engine/event_engine.h index 087f30b9855..e3bddb54c88 100644 --- a/include/grpc/event_engine/event_engine.h +++ b/include/grpc/event_engine/event_engine.h @@ -85,6 +85,7 @@ class EventEngine : public std::enable_shared_from_this { /// caller - the EventEngine will never delete a Closure, and upon /// cancellation, the EventEngine will simply forget the Closure exists. The /// caller is responsible for all necessary cleanup. + class Closure { public: Closure() = default; @@ -103,12 +104,14 @@ class EventEngine : public std::enable_shared_from_this { struct TaskHandle { intptr_t keys[2]; }; + static constexpr TaskHandle kInvalidTaskHandle{-1, -1}; /// A handle to a cancellable connection attempt. /// /// Returned by \a Connect, and can be passed to \a CancelConnect. struct ConnectionHandle { intptr_t keys[2]; }; + static constexpr ConnectionHandle kInvalidConnectionHandle{-1, -1}; /// Thin wrapper around a platform-specific sockaddr type. A sockaddr struct /// exists on all platforms that gRPC supports. /// diff --git a/package.xml b/package.xml index dc827fa0eb4..388ef994508 100644 --- a/package.xml +++ b/package.xml @@ -1021,6 +1021,7 @@ + diff --git a/src/core/BUILD b/src/core/BUILD index 87e08aa5118..4ccff69a079 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -54,6 +54,7 @@ grpc_cc_library( grpc_cc_library( name = "event_engine_common", srcs = [ + "lib/event_engine/event_engine.cc", "lib/event_engine/resolved_address.cc", "lib/event_engine/slice.cc", "lib/event_engine/slice_buffer.cc", diff --git a/src/core/lib/event_engine/event_engine.cc b/src/core/lib/event_engine/event_engine.cc new file mode 100644 index 00000000000..a81145912b5 --- /dev/null +++ b/src/core/lib/event_engine/event_engine.cc @@ -0,0 +1,25 @@ +// Copyright 2023 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +#include + +#include + +namespace grpc_event_engine { +namespace experimental { + +constexpr EventEngine::TaskHandle EventEngine::kInvalidTaskHandle; +constexpr EventEngine::ConnectionHandle EventEngine::kInvalidConnectionHandle; + +} // namespace experimental +} // namespace grpc_event_engine diff --git a/src/core/lib/event_engine/posix_engine/posix_engine.cc b/src/core/lib/event_engine/posix_engine/posix_engine.cc index 72ba60de7c6..d037a86a88c 100644 --- a/src/core/lib/event_engine/posix_engine/posix_engine.cc +++ b/src/core/lib/event_engine/posix_engine/posix_engine.cc @@ -232,7 +232,7 @@ EventEngine::ConnectionHandle PosixEventEngine::ConnectInternal( ep = absl::FailedPreconditionError(absl::StrCat( "connect failed: ", "invalid addr: ", addr_uri.value()))]() mutable { on_connect(std::move(ep)); }); - return {0, 0}; + return EventEngine::kInvalidConnectionHandle; } std::string name = absl::StrCat("tcp-client:", addr_uri.value()); @@ -253,7 +253,7 @@ EventEngine::ConnectionHandle PosixEventEngine::ConnectInternal( std::move(allocator), options)]() mutable { on_connect(std::move(ep)); }); - return {0, 0}; + return EventEngine::kInvalidConnectionHandle; } if (saved_errno != EWOULDBLOCK && saved_errno != EINPROGRESS) { // Connection already failed. Return 0 to discourage any cancellation @@ -265,7 +265,7 @@ EventEngine::ConnectionHandle PosixEventEngine::ConnectInternal( " error: ", std::strerror(saved_errno)))]() mutable { on_connect(std::move(ep)); }); - return {0, 0}; + return EventEngine::kInvalidConnectionHandle; } AsyncConnect* ac = new AsyncConnect( std::move(on_connect), shared_from_this(), executor_.get(), handle, @@ -554,7 +554,7 @@ EventEngine::ConnectionHandle PosixEventEngine::Connect( if (!socket.ok()) { Run([on_connect = std::move(on_connect), status = socket.status()]() mutable { on_connect(status); }); - return {0, 0}; + return EventEngine::kInvalidConnectionHandle; } return ConnectInternal((*socket).sock, std::move(on_connect), (*socket).mapped_target_addr, diff --git a/src/core/lib/event_engine/windows/windows_engine.cc b/src/core/lib/event_engine/windows/windows_engine.cc index c37e4f4040f..7391598c66f 100644 --- a/src/core/lib/event_engine/windows/windows_engine.cc +++ b/src/core/lib/event_engine/windows/windows_engine.cc @@ -216,7 +216,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect( Run([on_connect = std::move(on_connect), status = uri.status()]() mutable { on_connect(status); }); - return invalid_connection_handle; + return EventEngine::kInvalidConnectionHandle; } GRPC_EVENT_ENGINE_TRACE("EventEngine::%p connecting to %s", this, uri->c_str()); @@ -233,14 +233,14 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect( status = GRPC_WSA_ERROR(WSAGetLastError(), "WSASocket")]() mutable { on_connect(status); }); - return invalid_connection_handle; + return EventEngine::kInvalidConnectionHandle; } status = PrepareSocket(sock); if (!status.ok()) { Run([on_connect = std::move(on_connect), status]() mutable { on_connect(status); }); - return invalid_connection_handle; + return EventEngine::kInvalidConnectionHandle; } // Grab the function pointer for ConnectEx for that specific socket It may // change depending on the interface. @@ -257,7 +257,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect( "WSAIoctl(SIO_GET_EXTENSION_FUNCTION_POINTER)")]() mutable { on_connect(status); }); - return invalid_connection_handle; + return EventEngine::kInvalidConnectionHandle; } // bind the local address auto local_address = ResolvedAddressMakeWild6(0); @@ -267,7 +267,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect( status = GRPC_WSA_ERROR(WSAGetLastError(), "bind")]() mutable { on_connect(status); }); - return invalid_connection_handle; + return EventEngine::kInvalidConnectionHandle; } // Connect auto watched_socket = iocp_.Watch(sock); @@ -285,7 +285,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect( on_connect(status); }); watched_socket->MaybeShutdown(status); - return invalid_connection_handle; + return EventEngine::kInvalidConnectionHandle; } } GPR_ASSERT(watched_socket != nullptr); @@ -321,8 +321,8 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect( } bool WindowsEventEngine::CancelConnect(EventEngine::ConnectionHandle handle) { - if (TaskHandleComparator::Eq()(handle, - invalid_connection_handle)) { + if (TaskHandleComparator::Eq()( + handle, EventEngine::kInvalidConnectionHandle)) { GRPC_EVENT_ENGINE_TRACE("%s", "Attempted to cancel an invalid connection handle"); return false; diff --git a/src/core/lib/event_engine/windows/windows_engine.h b/src/core/lib/event_engine/windows/windows_engine.h index 00dca1135a2..66dd486dfea 100644 --- a/src/core/lib/event_engine/windows/windows_engine.h +++ b/src/core/lib/event_engine/windows/windows_engine.h @@ -45,10 +45,6 @@ namespace experimental { class WindowsEventEngine : public EventEngine, public grpc_core::KeepsGrpcInitialized { public: - constexpr static TaskHandle invalid_handle{-1, -1}; - constexpr static EventEngine::ConnectionHandle invalid_connection_handle{-1, - -1}; - class WindowsListener : public EventEngine::Listener { public: ~WindowsListener() override; diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index cb801eb771d..5e80cc57c64 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -481,6 +481,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/event_engine/channel_args_endpoint_config.cc', 'src/core/lib/event_engine/default_event_engine.cc', 'src/core/lib/event_engine/default_event_engine_factory.cc', + 'src/core/lib/event_engine/event_engine.cc', 'src/core/lib/event_engine/forkable.cc', 'src/core/lib/event_engine/memory_allocator.cc', 'src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc', diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 3652cac6bda..9bd60293755 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -2033,6 +2033,7 @@ src/core/lib/event_engine/default_event_engine.cc \ src/core/lib/event_engine/default_event_engine.h \ src/core/lib/event_engine/default_event_engine_factory.cc \ src/core/lib/event_engine/default_event_engine_factory.h \ +src/core/lib/event_engine/event_engine.cc \ src/core/lib/event_engine/executor/executor.h \ src/core/lib/event_engine/forkable.cc \ src/core/lib/event_engine/forkable.h \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index edaac2095b4..6740bde46d1 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1812,6 +1812,7 @@ src/core/lib/event_engine/default_event_engine.cc \ src/core/lib/event_engine/default_event_engine.h \ src/core/lib/event_engine/default_event_engine_factory.cc \ src/core/lib/event_engine/default_event_engine_factory.h \ +src/core/lib/event_engine/event_engine.cc \ src/core/lib/event_engine/executor/executor.h \ src/core/lib/event_engine/forkable.cc \ src/core/lib/event_engine/forkable.h \