[EventEngine] Add EventEngine::*Handle equality operators (#32695)

This allows us to replace `absl::optional<TaskHandle>` with checks
against the invalid handle.

This PR also replaces the differently-named invalid handle instances
with a uniform way of accessing static invalid instances across all
handle types, which aids a bit in testing.

CMakeLists.txt

@@ -997,6 +997,7 @@ if(gRPC_BUILD_TESTS)
   add_dependencies(buildtests_cxx h2_ssl_cert_test)
   add_dependencies(buildtests_cxx h2_ssl_session_reuse_test)
   add_dependencies(buildtests_cxx h2_tls_peer_property_external_verifier_test)
+  add_dependencies(buildtests_cxx handle_tests)
   if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX)
     add_dependencies(buildtests_cxx handshake_server_with_readahead_handshaker_test)
   endif()
@@ -13186,6 +13187,43 @@ target_link_libraries(h2_tls_peer_property_external_verifier_test
 )
 
 
+endif()
+if(gRPC_BUILD_TESTS)
+
+add_executable(handle_tests
+  test/core/event_engine/handle_tests.cc
+  third_party/googletest/googletest/src/gtest-all.cc
+  third_party/googletest/googlemock/src/gmock-all.cc
+)
+target_compile_features(handle_tests PUBLIC cxx_std_14)
+target_include_directories(handle_tests
+  PRIVATE
+    ${CMAKE_CURRENT_SOURCE_DIR}
+    ${CMAKE_CURRENT_SOURCE_DIR}/include
+    ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR}
+    ${_gRPC_RE2_INCLUDE_DIR}
+    ${_gRPC_SSL_INCLUDE_DIR}
+    ${_gRPC_UPB_GENERATED_DIR}
+    ${_gRPC_UPB_GRPC_GENERATED_DIR}
+    ${_gRPC_UPB_INCLUDE_DIR}
+    ${_gRPC_XXHASH_INCLUDE_DIR}
+    ${_gRPC_ZLIB_INCLUDE_DIR}
+    third_party/googletest/googletest/include
+    third_party/googletest/googletest
+    third_party/googletest/googlemock/include
+    third_party/googletest/googlemock
+    ${_gRPC_PROTO_GENS_DIR}
+)
+
+target_link_libraries(handle_tests
+  ${_gRPC_BASELIB_LIBRARIES}
+  ${_gRPC_PROTOBUF_LIBRARIES}
+  ${_gRPC_ZLIB_LIBRARIES}
+  ${_gRPC_ALLTARGETS_LIBRARIES}
+  grpc
+)
+
+
 endif()
 if(gRPC_BUILD_TESTS)
 if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX)

build_autogenerated.yaml

@@ -8513,6 +8513,16 @@ targets:
   - test/core/end2end/h2_tls_peer_property_external_verifier_test.cc
   deps:
   - grpc_test_util
+- name: handle_tests
+  gtest: true
+  build: test
+  language: c++
+  headers: []
+  src:
+  - test/core/event_engine/handle_tests.cc
+  deps:
+  - grpc
+  uses_polling: false
 - name: handshake_server_with_readahead_handshaker_test
   gtest: true
   build: test

include/grpc/event_engine/event_engine.h

@@ -122,15 +122,21 @@ class EventEngine : public std::enable_shared_from_this<EventEngine> {
   /// \a Cancel method.
   struct TaskHandle {
     intptr_t keys[2];
+    static const TaskHandle kInvalid;
+    friend bool operator==(const TaskHandle& lhs, const TaskHandle& rhs);
+    friend bool operator!=(const TaskHandle& lhs, const TaskHandle& rhs);
   };
-  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 const ConnectionHandle kInvalid;
+    friend bool operator==(const ConnectionHandle& lhs,
+                           const ConnectionHandle& rhs);
+    friend bool operator!=(const ConnectionHandle& lhs,
+                           const ConnectionHandle& rhs);
   };
-  static constexpr ConnectionHandle kInvalidConnectionHandle{-1, -1};
   /// Thin wrapper around a platform-specific sockaddr type. A sockaddr struct
   /// exists on all platforms that gRPC supports.
   ///
@@ -319,6 +325,11 @@ class EventEngine : public std::enable_shared_from_this<EventEngine> {
     /// Task handle for DNS Resolution requests.
     struct LookupTaskHandle {
       intptr_t keys[2];
+      static const LookupTaskHandle kInvalid;
+      friend bool operator==(const LookupTaskHandle& lhs,
+                             const LookupTaskHandle& rhs);
+      friend bool operator!=(const LookupTaskHandle& lhs,
+                             const LookupTaskHandle& rhs);
     };
     /// Optional configuration for DNSResolvers.
     struct ResolverOptions {

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc

@@ -34,6 +34,7 @@
 #include "absl/strings/strip.h"
 #include "absl/types/optional.h"
 
+#include <grpc/event_engine/event_engine.h>
 #include <grpc/grpc.h>
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>

src/core/lib/event_engine/event_engine.cc

@@ -13,13 +13,48 @@
 // limitations under the License.
 #include <grpc/support/port_platform.h>
 
+#include <stdint.h>
+
 #include <grpc/event_engine/event_engine.h>
 
 namespace grpc_event_engine {
 namespace experimental {
 
-constexpr EventEngine::TaskHandle EventEngine::kInvalidTaskHandle;
-constexpr EventEngine::ConnectionHandle EventEngine::kInvalidConnectionHandle;
+constexpr EventEngine::TaskHandle EventEngine::TaskHandle::kInvalid = {-1, -1};
+constexpr EventEngine::ConnectionHandle
+    EventEngine::ConnectionHandle::kInvalid = {-1, -1};
+constexpr EventEngine::DNSResolver::LookupTaskHandle
+    EventEngine::DNSResolver::LookupTaskHandle::kInvalid = {-1, -1};
+
+bool operator==(const EventEngine::TaskHandle& lhs,
+                const EventEngine::TaskHandle& rhs) {
+  return lhs.keys[0] == rhs.keys[0] && lhs.keys[1] == rhs.keys[1];
+}
+
+bool operator!=(const EventEngine::TaskHandle& lhs,
+                const EventEngine::TaskHandle& rhs) {
+  return !(lhs == rhs);
+}
+
+bool operator==(const EventEngine::ConnectionHandle& lhs,
+                const EventEngine::ConnectionHandle& rhs) {
+  return lhs.keys[0] == rhs.keys[0] && lhs.keys[1] == rhs.keys[1];
+}
+
+bool operator!=(const EventEngine::ConnectionHandle& lhs,
+                const EventEngine::ConnectionHandle& rhs) {
+  return !(lhs == rhs);
+}
+
+bool operator==(const EventEngine::DNSResolver::LookupTaskHandle& lhs,
+                const EventEngine::DNSResolver::LookupTaskHandle& rhs) {
+  return lhs.keys[0] == rhs.keys[0] && lhs.keys[1] == rhs.keys[1];
+}
+
+bool operator!=(const EventEngine::DNSResolver::LookupTaskHandle& lhs,
+                const EventEngine::DNSResolver::LookupTaskHandle& rhs) {
+  return !(lhs == rhs);
+}
 
 }  // namespace experimental
 }  // namespace grpc_event_engine

src/core/lib/event_engine/handle_containers.h

@@ -39,34 +39,19 @@ struct TaskHandleComparator {
       return absl::Hash<HashType>()({handle.keys[0], handle.keys[1]});
     }
   };
-  struct Eq {
-    using is_transparent = void;
-    bool operator()(const TaskHandle& lhs, const TaskHandle& rhs) const {
-      return lhs.keys[0] == rhs.keys[0] && lhs.keys[1] == rhs.keys[1];
-    }
-  };
 };
 
-using TaskHandleSet = absl::flat_hash_set<
-    grpc_event_engine::experimental::EventEngine::TaskHandle,
-    TaskHandleComparator<
-        grpc_event_engine::experimental::EventEngine::TaskHandle>::Hash,
-    TaskHandleComparator<
-        grpc_event_engine::experimental::EventEngine::TaskHandle>::Eq>;
+using TaskHandleSet =
+    absl::flat_hash_set<EventEngine::TaskHandle,
+                        TaskHandleComparator<EventEngine::TaskHandle>::Hash>;
 
 using ConnectionHandleSet = absl::flat_hash_set<
-    grpc_event_engine::experimental::EventEngine::ConnectionHandle,
-    TaskHandleComparator<
-        grpc_event_engine::experimental::EventEngine::ConnectionHandle>::Hash,
-    TaskHandleComparator<
-        grpc_event_engine::experimental::EventEngine::ConnectionHandle>::Eq>;
+    EventEngine::ConnectionHandle,
+    TaskHandleComparator<EventEngine::ConnectionHandle>::Hash>;
 
 using LookupTaskHandleSet = absl::flat_hash_set<
-    grpc_event_engine::experimental::EventEngine::DNSResolver::LookupTaskHandle,
-    TaskHandleComparator<grpc_event_engine::experimental::EventEngine::
-                             DNSResolver::LookupTaskHandle>::Hash,
-    TaskHandleComparator<grpc_event_engine::experimental::EventEngine::
-                             DNSResolver::LookupTaskHandle>::Eq>;
+    EventEngine::DNSResolver::LookupTaskHandle,
+    TaskHandleComparator<EventEngine::DNSResolver::LookupTaskHandle>::Hash>;
 
 }  // namespace experimental
 }  // namespace grpc_event_engine

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 EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
 
   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 EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
   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 EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
   AsyncConnect* ac = new AsyncConnect(
       std::move(on_connect), shared_from_this(), executor_.get(), handle,
@@ -560,7 +560,7 @@ EventEngine::ConnectionHandle PosixEventEngine::Connect(
   if (!socket.ok()) {
     Run([on_connect = std::move(on_connect),
          status = socket.status()]() mutable { on_connect(status); });
-    return EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
   return ConnectInternal((*socket).sock, std::move(on_connect),
                          (*socket).mapped_target_addr,

src/core/lib/event_engine/windows/windows_engine.cc

@@ -226,7 +226,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect(
     Run([on_connect = std::move(on_connect), status = uri.status()]() mutable {
       on_connect(status);
     });
-    return EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
   GRPC_EVENT_ENGINE_TRACE("EventEngine::%p connecting to %s", this,
                           uri->c_str());
@@ -243,14 +243,14 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect(
          status = GRPC_WSA_ERROR(WSAGetLastError(), "WSASocket")]() mutable {
       on_connect(status);
     });
-    return EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
   status = PrepareSocket(sock);
   if (!status.ok()) {
     Run([on_connect = std::move(on_connect), status]() mutable {
       on_connect(status);
     });
-    return EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
   // Grab the function pointer for ConnectEx for that specific socket It may
   // change depending on the interface.
@@ -267,7 +267,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect(
              "WSAIoctl(SIO_GET_EXTENSION_FUNCTION_POINTER)")]() mutable {
       on_connect(status);
     });
-    return EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
   // bind the local address
   auto local_address = ResolvedAddressMakeWild6(0);
@@ -277,7 +277,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect(
          status = GRPC_WSA_ERROR(WSAGetLastError(), "bind")]() mutable {
       on_connect(status);
     });
-    return EventEngine::kInvalidConnectionHandle;
+    return EventEngine::ConnectionHandle::kInvalid;
   }
   // Connect
   auto watched_socket = iocp_.Watch(sock);
@@ -295,7 +295,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect(
         on_connect(status);
       });
       watched_socket->Shutdown(DEBUG_LOCATION, "ConnectEx");
-      return EventEngine::kInvalidConnectionHandle;
+      return EventEngine::ConnectionHandle::kInvalid;
     }
   }
   GPR_ASSERT(watched_socket != nullptr);
@@ -331,8 +331,7 @@ EventEngine::ConnectionHandle WindowsEventEngine::Connect(
 }
 
 bool WindowsEventEngine::CancelConnect(EventEngine::ConnectionHandle handle) {
-  if (TaskHandleComparator<ConnectionHandle>::Eq()(
-          handle, EventEngine::kInvalidConnectionHandle)) {
+  if (handle == EventEngine::ConnectionHandle::kInvalid) {
     GRPC_EVENT_ENGINE_TRACE("%s",
                             "Attempted to cancel an invalid connection handle");
     return false;

test/core/event_engine/BUILD

@@ -190,6 +190,19 @@ grpc_cc_test(
     ],
 )
 
+grpc_cc_test(
+    name = "handle_tests",
+    srcs = ["handle_tests.cc"],
+    external_deps = ["gtest"],
+    language = "C++",
+    uses_event_engine = False,
+    uses_polling = False,
+    deps = [
+        "//:gpr_platform",
+        "//:grpc",
+    ],
+)
+
 grpc_cc_library(
     name = "event_engine_test_utils",
     testonly = True,

test/core/event_engine/handle_tests.cc

@@ -0,0 +1,52 @@
+// 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 <grpc/support/port_platform.h>
+
+#include "gtest/gtest.h"
+
+#include <grpc/event_engine/event_engine.h>
+
+using ::grpc_event_engine::experimental::EventEngine;
+
+template <typename T>
+class TaskHandleTest : public testing::Test {};
+
+using HandleTypes =
+    ::testing::Types<EventEngine::TaskHandle, EventEngine::ConnectionHandle,
+                     EventEngine::DNSResolver::LookupTaskHandle>;
+TYPED_TEST_SUITE(TaskHandleTest, HandleTypes);
+
+TYPED_TEST(TaskHandleTest, Identity) {
+  TypeParam t{42, 43};
+  ASSERT_EQ(t, t);
+}
+
+TYPED_TEST(TaskHandleTest, CommutativeEquality) {
+  TypeParam t1{42, 43};
+  TypeParam t2 = t1;
+  ASSERT_EQ(t1, t2);
+  ASSERT_EQ(t2, t1);
+}
+
+TYPED_TEST(TaskHandleTest, Validity) {
+  TypeParam t{42, 43};
+  ASSERT_NE(t, TypeParam::kInvalid);
+  ASSERT_NE(TypeParam::kInvalid, t);
+  ASSERT_EQ(TypeParam::kInvalid, TypeParam::kInvalid);
+}
+
+int main(int argc, char** argv) {
+  testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}

tools/run_tests/generated/tests.json

@@ -3791,6 +3791,30 @@
     ],
     "uses_polling": true
   },
+  {
+    "args": [],
+    "benchmark": false,
+    "ci_platforms": [
+      "linux",
+      "mac",
+      "posix",
+      "windows"
+    ],
+    "cpu_cost": 1.0,
+    "exclude_configs": [],
+    "exclude_iomgrs": [],
+    "flaky": false,
+    "gtest": true,
+    "language": "c++",
+    "name": "handle_tests",
+    "platforms": [
+      "linux",
+      "mac",
+      "posix",
+      "windows"
+    ],
+    "uses_polling": false
+  },
   {
     "args": [],
     "benchmark": false,