From df435e584dbd60e726c2bad5bb6f164ec883d850 Mon Sep 17 00:00:00 2001 From: Guantao Liu Date: Mon, 2 Dec 2019 15:32:34 -0800 Subject: [PATCH] Address review comments. --- CMakeLists.txt | 65 +++++++------- Makefile | 84 +++++++++++-------- build.yaml | 21 ++--- .../lib/iomgr/poller/eventmanager_libuv.cc | 19 +++-- .../lib/iomgr/poller/eventmanager_libuv.h | 6 +- test/core/iomgr/poller/BUILD | 3 + .../iomgr/poller/eventmanager_libuv_test.cc | 35 ++++++-- tools/run_tests/generated/tests.json | 48 +++++------ 8 files changed, 165 insertions(+), 116 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 038704bd76b..5d3ffc6bea8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -394,7 +394,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) @@ -734,6 +733,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) @@ -6976,33 +6976,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 ) @@ -12557,6 +12530,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 index d3aa0b488bd..a79b6820355 100644 --- 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 @@ -1474,7 +1474,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 \ @@ -1700,6 +1699,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 \ @@ -1871,6 +1871,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 \ @@ -2033,8 +2034,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" @@ -2373,6 +2372,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" @@ -9985,38 +9986,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 \ @@ -16747,6 +16716,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 index 86fd7a0c942..39a010cae6c 100644 --- a/build.yaml +++ b/build.yaml @@ -2593,16 +2593,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 @@ -4922,6 +4912,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 index bf772140032..82f35e4805b 100644 --- a/src/core/lib/iomgr/poller/eventmanager_libuv.cc +++ b/src/core/lib/iomgr/poller/eventmanager_libuv.cc @@ -27,14 +27,15 @@ 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(em)->RunWorkerLoop(); }, this); workers_.back().Start(); } @@ -42,15 +43,15 @@ grpc::experimental::LibuvEventManager::LibuvEventManager(const Options& options) 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(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 index 25698ee19ff..c6f3256d62b 100644 --- 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 should_stop_; + grpc_core::Atomic should_stop_ = false; // A refcount preventing the EventManager from shutdown. - grpc_core::Atomic shutdown_refcount_; + grpc_core::Atomic shutdown_refcount_ = 0; // Worker threads of the EventManager. std::vector workers_; }; diff --git a/test/core/iomgr/poller/BUILD b/test/core/iomgr/poller/BUILD index 53e3288abaa..276d75e9f03 100644 --- 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 = [ diff --git a/test/core/iomgr/poller/eventmanager_libuv_test.cc b/test/core/iomgr/poller/eventmanager_libuv_test.cc index 544b4ff2808..c772d5cd949 100644 --- a/test/core/iomgr/poller/eventmanager_libuv_test.cc +++ b/test/core/iomgr/poller/eventmanager_libuv_test.cc @@ -21,12 +21,16 @@ #include #include +#include #include "test/core/util/test_config.h" using grpc::experimental::LibuvEventManager; -static void test_manager_allocation() { +namespace grpc_core { +namespace { + +TEST(LibuvEventManager, Allocation) { for (int i = 0; i < 10; i++) { LibuvEventManager* em = new LibuvEventManager(i); gpr_sleep_until(grpc_timeout_milliseconds_to_deadline(1)); @@ -34,7 +38,7 @@ static void test_manager_allocation() { } } -static void test_shutdown_ref() { +TEST(LibuvEventManager, ShutdownRef) { for (int i = 0; i < 10; i++) { LibuvEventManager* em = new LibuvEventManager(i); for (int j = 0; j < i; j++) { @@ -48,11 +52,30 @@ static void test_shutdown_ref() { } } +TEST(LibuvEventManager, ShutdownRefAsync) { + for (int i = 0; i < 10; i++) { + LibuvEventManager* em = new LibuvEventManager(i); + for (int j = 0; j < i; j++) { + em->ShutdownRef(); + } + grpc_core::Thread deleter("deleter", [em](void*) { delete em; }, nullptr); + deleter.Start(); + gpr_sleep_until(grpc_timeout_milliseconds_to_deadline(1)); + for (int j = 0; j < i; j++) { + em->ShutdownUnref(); + } + deleter.Join(); + } +} + +} // namespace +} // namespace grpc_core + int main(int argc, char** argv) { - grpc::testing::TestEnvironment env(argc, argv); grpc_init(); - test_manager_allocation(); - test_shutdown_ref(); + grpc::testing::TestEnvironment env(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + int retval = RUN_ALL_TESTS(); grpc_shutdown(); - return 0; + return retval; } diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index cd93a2ee76f..aeadb470dba 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -653,30 +653,6 @@ ], "uses_polling": true }, - { - "args": [], - "benchmark": false, - "ci_platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "cpu_cost": 1.0, - "exclude_configs": [], - "exclude_iomgrs": [], - "flaky": false, - "gtest": false, - "language": "c", - "name": "eventmanager_libuv_test", - "platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "uses_polling": false - }, { "args": [], "benchmark": false, @@ -4565,6 +4541,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": "eventmanager_libuv_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": false + }, { "args": [], "benchmark": false,