Address review comments.

mohits · Dec 3, 2019 · e2b53be · e2b53be
1 parent 1de3076
commit e2b53be
Show file tree

Hide file tree

Showing 8 changed files with 165 additions and 116 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -399,7 +399,6 @@ if(gRPC_BUILD_TESTS)
   if(_gRPC_PLATFORM_LINUX)
     add_dependencies(buildtests_c ev_epollex_linux_test)
   endif()
-  add_dependencies(buildtests_c eventmanager_libuv_test)
   add_dependencies(buildtests_c fake_resolver_test)
   if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX)
     add_dependencies(buildtests_c fake_transport_security_test)
@@ -739,6 +738,7 @@ if(gRPC_BUILD_TESTS)
   add_dependencies(buildtests_cxx delegating_channel_test)
   add_dependencies(buildtests_cxx end2end_test)
   add_dependencies(buildtests_cxx error_details_test)
+  add_dependencies(buildtests_cxx eventmanager_libuv_test)
   add_dependencies(buildtests_cxx exception_test)
   add_dependencies(buildtests_cxx filter_end2end_test)
   add_dependencies(buildtests_cxx generic_end2end_test)
@@ -6986,33 +6986,6 @@ endif()
 endif()
 if(gRPC_BUILD_TESTS)
 
-add_executable(eventmanager_libuv_test
-  test/core/iomgr/poller/eventmanager_libuv_test.cc
-)
-
-target_include_directories(eventmanager_libuv_test
-  PRIVATE
-    ${CMAKE_CURRENT_SOURCE_DIR}
-    ${CMAKE_CURRENT_SOURCE_DIR}/include
-    ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR}
-    ${_gRPC_SSL_INCLUDE_DIR}
-    ${_gRPC_UPB_GENERATED_DIR}
-    ${_gRPC_UPB_GRPC_GENERATED_DIR}
-    ${_gRPC_UPB_INCLUDE_DIR}
-    ${_gRPC_ZLIB_INCLUDE_DIR}
-)
-
-target_link_libraries(eventmanager_libuv_test
-  ${_gRPC_ALLTARGETS_LIBRARIES}
-  grpc_test_util
-  grpc
-  gpr
-)
-
-
-endif()
-if(gRPC_BUILD_TESTS)
-
 add_executable(fake_resolver_test
   test/core/client_channel/resolvers/fake_resolver_test.cc
 )
@@ -12567,6 +12540,42 @@ target_link_libraries(error_details_test
 )
 
 
+endif()
+if(gRPC_BUILD_TESTS)
+
+add_executable(eventmanager_libuv_test
+  test/core/iomgr/poller/eventmanager_libuv_test.cc
+  third_party/googletest/googletest/src/gtest-all.cc
+  third_party/googletest/googlemock/src/gmock-all.cc
+)
+
+target_include_directories(eventmanager_libuv_test
+  PRIVATE
+    ${CMAKE_CURRENT_SOURCE_DIR}
+    ${CMAKE_CURRENT_SOURCE_DIR}/include
+    ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR}
+    ${_gRPC_SSL_INCLUDE_DIR}
+    ${_gRPC_UPB_GENERATED_DIR}
+    ${_gRPC_UPB_GRPC_GENERATED_DIR}
+    ${_gRPC_UPB_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(eventmanager_libuv_test
+  ${_gRPC_PROTOBUF_LIBRARIES}
+  ${_gRPC_ALLTARGETS_LIBRARIES}
+  grpc_test_util
+  grpc
+  gpr
+  ${_gRPC_GFLAGS_LIBRARIES}
+)
+
+
 endif()
 if(gRPC_BUILD_TESTS)
 

diff --git a/Makefile b/Makefile
@@ -1036,7 +1036,6 @@ dualstack_socket_test: $(BINDIR)/$(CONFIG)/dualstack_socket_test
 endpoint_pair_test: $(BINDIR)/$(CONFIG)/endpoint_pair_test
 error_test: $(BINDIR)/$(CONFIG)/error_test
 ev_epollex_linux_test: $(BINDIR)/$(CONFIG)/ev_epollex_linux_test
-eventmanager_libuv_test: $(BINDIR)/$(CONFIG)/eventmanager_libuv_test
 fake_resolver_test: $(BINDIR)/$(CONFIG)/fake_resolver_test
 fake_transport_security_test: $(BINDIR)/$(CONFIG)/fake_transport_security_test
 fd_conservation_posix_test: $(BINDIR)/$(CONFIG)/fd_conservation_posix_test
@@ -1220,6 +1219,7 @@ cxx_time_test: $(BINDIR)/$(CONFIG)/cxx_time_test
 delegating_channel_test: $(BINDIR)/$(CONFIG)/delegating_channel_test
 end2end_test: $(BINDIR)/$(CONFIG)/end2end_test
 error_details_test: $(BINDIR)/$(CONFIG)/error_details_test
+eventmanager_libuv_test: $(BINDIR)/$(CONFIG)/eventmanager_libuv_test
 exception_test: $(BINDIR)/$(CONFIG)/exception_test
 filter_end2end_test: $(BINDIR)/$(CONFIG)/filter_end2end_test
 gen_hpack_tables: $(BINDIR)/$(CONFIG)/gen_hpack_tables
@@ -1476,7 +1476,6 @@ buildtests_c: privatelibs_c \
   $(BINDIR)/$(CONFIG)/endpoint_pair_test \
   $(BINDIR)/$(CONFIG)/error_test \
   $(BINDIR)/$(CONFIG)/ev_epollex_linux_test \
-  $(BINDIR)/$(CONFIG)/eventmanager_libuv_test \
   $(BINDIR)/$(CONFIG)/fake_resolver_test \
   $(BINDIR)/$(CONFIG)/fake_transport_security_test \
   $(BINDIR)/$(CONFIG)/fd_conservation_posix_test \
@@ -1702,6 +1701,7 @@ buildtests_cxx: privatelibs_cxx \
   $(BINDIR)/$(CONFIG)/delegating_channel_test \
   $(BINDIR)/$(CONFIG)/end2end_test \
   $(BINDIR)/$(CONFIG)/error_details_test \
+  $(BINDIR)/$(CONFIG)/eventmanager_libuv_test \
   $(BINDIR)/$(CONFIG)/exception_test \
   $(BINDIR)/$(CONFIG)/filter_end2end_test \
   $(BINDIR)/$(CONFIG)/generic_end2end_test \
@@ -1875,6 +1875,7 @@ buildtests_cxx: privatelibs_cxx \
   $(BINDIR)/$(CONFIG)/delegating_channel_test \
   $(BINDIR)/$(CONFIG)/end2end_test \
   $(BINDIR)/$(CONFIG)/error_details_test \
+  $(BINDIR)/$(CONFIG)/eventmanager_libuv_test \
   $(BINDIR)/$(CONFIG)/exception_test \
   $(BINDIR)/$(CONFIG)/filter_end2end_test \
   $(BINDIR)/$(CONFIG)/generic_end2end_test \
@@ -2039,8 +2040,6 @@ test_c: buildtests_c
 	$(Q) $(BINDIR)/$(CONFIG)/error_test || ( echo test error_test failed ; exit 1 )
 	$(E) "[RUN]     Testing ev_epollex_linux_test"
 	$(Q) $(BINDIR)/$(CONFIG)/ev_epollex_linux_test || ( echo test ev_epollex_linux_test failed ; exit 1 )
-	$(E) "[RUN]     Testing eventmanager_libuv_test"
-	$(Q) $(BINDIR)/$(CONFIG)/eventmanager_libuv_test || ( echo test eventmanager_libuv_test failed ; exit 1 )
 	$(E) "[RUN]     Testing fake_resolver_test"
 	$(Q) $(BINDIR)/$(CONFIG)/fake_resolver_test || ( echo test fake_resolver_test failed ; exit 1 )
 	$(E) "[RUN]     Testing fake_transport_security_test"
@@ -2379,6 +2378,8 @@ test_cxx: buildtests_cxx
 	$(Q) $(BINDIR)/$(CONFIG)/end2end_test || ( echo test end2end_test failed ; exit 1 )
 	$(E) "[RUN]     Testing error_details_test"
 	$(Q) $(BINDIR)/$(CONFIG)/error_details_test || ( echo test error_details_test failed ; exit 1 )
+	$(E) "[RUN]     Testing eventmanager_libuv_test"
+	$(Q) $(BINDIR)/$(CONFIG)/eventmanager_libuv_test || ( echo test eventmanager_libuv_test failed ; exit 1 )
 	$(E) "[RUN]     Testing exception_test"
 	$(Q) $(BINDIR)/$(CONFIG)/exception_test || ( echo test exception_test failed ; exit 1 )
 	$(E) "[RUN]     Testing filter_end2end_test"
@@ -9994,38 +9995,6 @@ endif
 endif
 
 
-EVENTMANAGER_LIBUV_TEST_SRC = \
-    test/core/iomgr/poller/eventmanager_libuv_test.cc \
-
-EVENTMANAGER_LIBUV_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(EVENTMANAGER_LIBUV_TEST_SRC))))
-ifeq ($(NO_SECURE),true)
-
-# You can't build secure targets if you don't have OpenSSL.
-
-$(BINDIR)/$(CONFIG)/eventmanager_libuv_test: openssl_dep_error
-
-else
-
-
-
-$(BINDIR)/$(CONFIG)/eventmanager_libuv_test: $(EVENTMANAGER_LIBUV_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a
-	$(E) "[LD]      Linking $@"
-	$(Q) mkdir -p `dirname $@`
-	$(Q) $(LDXX) $(LDFLAGS) $(EVENTMANAGER_LIBUV_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBS) $(LDLIBS_SECURE) -o $(BINDIR)/$(CONFIG)/eventmanager_libuv_test
-
-endif
-
-$(OBJDIR)/$(CONFIG)/test/core/iomgr/poller/eventmanager_libuv_test.o:  $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a
-
-deps_eventmanager_libuv_test: $(EVENTMANAGER_LIBUV_TEST_OBJS:.o=.dep)
-
-ifneq ($(NO_SECURE),true)
-ifneq ($(NO_DEPS),true)
--include $(EVENTMANAGER_LIBUV_TEST_OBJS:.o=.dep)
-endif
-endif
-
-
 FAKE_RESOLVER_TEST_SRC = \
     test/core/client_channel/resolvers/fake_resolver_test.cc \
 
@@ -16756,6 +16725,49 @@ endif
 $(OBJDIR)/$(CONFIG)/test/cpp/util/error_details_test.o: $(GENDIR)/src/proto/grpc/testing/echo_messages.pb.cc $(GENDIR)/src/proto/grpc/testing/echo_messages.grpc.pb.cc
 
 
+EVENTMANAGER_LIBUV_TEST_SRC = \
+    test/core/iomgr/poller/eventmanager_libuv_test.cc \
+
+EVENTMANAGER_LIBUV_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(EVENTMANAGER_LIBUV_TEST_SRC))))
+ifeq ($(NO_SECURE),true)
+
+# You can't build secure targets if you don't have OpenSSL.
+
+$(BINDIR)/$(CONFIG)/eventmanager_libuv_test: openssl_dep_error
+
+else
+
+
+
+
+ifeq ($(NO_PROTOBUF),true)
+
+# You can't build the protoc plugins or protobuf-enabled targets if you don't have protobuf 3.5.0+.
+
+$(BINDIR)/$(CONFIG)/eventmanager_libuv_test: protobuf_dep_error
+
+else
+
+$(BINDIR)/$(CONFIG)/eventmanager_libuv_test: $(PROTOBUF_DEP) $(EVENTMANAGER_LIBUV_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a
+	$(E) "[LD]      Linking $@"
+	$(Q) mkdir -p `dirname $@`
+	$(Q) $(LDXX) $(LDFLAGS) $(EVENTMANAGER_LIBUV_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/eventmanager_libuv_test
+
+endif
+
+endif
+
+$(OBJDIR)/$(CONFIG)/test/core/iomgr/poller/eventmanager_libuv_test.o:  $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a
+
+deps_eventmanager_libuv_test: $(EVENTMANAGER_LIBUV_TEST_OBJS:.o=.dep)
+
+ifneq ($(NO_SECURE),true)
+ifneq ($(NO_DEPS),true)
+-include $(EVENTMANAGER_LIBUV_TEST_OBJS:.o=.dep)
+endif
+endif
+
+
 EXCEPTION_TEST_SRC = \
     test/cpp/end2end/exception_test.cc \
 

diff --git a/build.yaml b/build.yaml
@@ -2596,16 +2596,6 @@ targets:
   - uv
   platforms:
   - linux
-- name: eventmanager_libuv_test
-  build: test
-  language: c
-  src:
-  - test/core/iomgr/poller/eventmanager_libuv_test.cc
-  deps:
-  - grpc_test_util
-  - grpc
-  - gpr
-  uses_polling: false
 - name: fake_resolver_test
   build: test
   language: c
@@ -4925,6 +4915,17 @@ targets:
   deps:
   - grpc++_error_details
   - grpc++
+- name: eventmanager_libuv_test
+  gtest: true
+  build: test
+  language: c++
+  src:
+  - test/core/iomgr/poller/eventmanager_libuv_test.cc
+  deps:
+  - grpc_test_util
+  - grpc
+  - gpr
+  uses_polling: false
 - name: exception_test
   gtest: true
   build: test

diff --git a/src/core/lib/iomgr/poller/eventmanager_libuv.cc b/src/core/lib/iomgr/poller/eventmanager_libuv.cc
@@ -27,30 +27,31 @@ grpc::experimental::LibuvEventManager::Options::Options(int num_workers)
     : num_workers_(num_workers) {}
 
 grpc::experimental::LibuvEventManager::LibuvEventManager(const Options& options)
-    : options_(options), should_stop_(0), shutdown_refcount_(0) {
+    : options_(options) {
   int num_workers = options_.num_workers();
+  // Number of workers can't be 0 if we do not accept thread donation.
   // TODO(guantaol): replaces the hard-coded number with a flag.
-  if (num_workers < 0) num_workers = 32;
+  if (num_workers <= 0) num_workers = 32;
 
   for (int i = 0; i < num_workers; i++) {
     workers_.emplace_back(options_.thread_name_prefix().c_str(),
-                          &grpc::experimental::LibuvEventManager::RunWorkerLoop,
+                          [](void* em) { static_cast<LibuvEventManager*>(em)->RunWorkerLoop(); },
                           this);
     workers_.back().Start();
   }
 }
 
 grpc::experimental::LibuvEventManager::~LibuvEventManager() {
   Shutdown();
-  for (auto it = workers_.begin(); it != workers_.end(); it++) {
-    it->Join();
+  for (auto& th : workers_) {
+    th.Join();
   }
 }
 
-void grpc::experimental::LibuvEventManager::RunWorkerLoop(void* manager) {
-  LibuvEventManager* event_manager = static_cast<LibuvEventManager*>(manager);
+void grpc::experimental::LibuvEventManager::RunWorkerLoop() {
   while (true) {
-    if (event_manager->ShouldStop()) return;
+    // TODO(guantaol): extend the worker loop with real work.
+    if (ShouldStop()) return;
     gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC),
                                  gpr_time_from_micros(10, GPR_TIMESPAN)));
   }
@@ -65,7 +66,7 @@ void grpc::experimental::LibuvEventManager::Shutdown() {
     return;  // Already shut down.
   while (shutdown_refcount_.Load(grpc_core::MemoryOrder::ACQUIRE) > 0)
     ;
-  should_stop_.Store(1, grpc_core::MemoryOrder::RELEASE);
+  should_stop_.Store(true, grpc_core::MemoryOrder::RELEASE);
 }
 
 void grpc::experimental::LibuvEventManager::ShutdownRef() {

diff --git a/src/core/lib/iomgr/poller/eventmanager_libuv.h b/src/core/lib/iomgr/poller/eventmanager_libuv.h
@@ -64,16 +64,16 @@ class LibuvEventManager {
 
  private:
   // Function run by the worker threads.
-  static void RunWorkerLoop(void* manager);
+  void RunWorkerLoop();
 
   // Whether the EventManager has been shut down.
   bool ShouldStop();
 
   const Options options_;
   // Whether the EventManager workers should be stopped.
-  grpc_core::Atomic<int> should_stop_;
+  grpc_core::Atomic<bool> should_stop_ = false;
   // A refcount preventing the EventManager from shutdown.
-  grpc_core::Atomic<int> shutdown_refcount_;
+  grpc_core::Atomic<int> shutdown_refcount_ = 0;
   // Worker threads of the EventManager.
   std::vector<grpc_core::Thread> workers_;
 };

diff --git a/test/core/iomgr/poller/BUILD b/test/core/iomgr/poller/BUILD
@@ -25,6 +25,9 @@ grpc_package(
 grpc_cc_test(
     name = "eventmanager_libuv_test",
     srcs = ["eventmanager_libuv_test.cc"],
+    external_deps = [
+        "gtest",
+    ],
     language = "C++",
     uses_polling = False,
     deps = [