Merge pull request #18547 from lidizheng/fix-gevent

Fix gevent segfault with C-Ares as DNS resolver

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

@@ -43,6 +43,7 @@
 #include "src/core/lib/gprpp/manual_constructor.h"
 #include "src/core/lib/iomgr/combiner.h"
 #include "src/core/lib/iomgr/gethostname.h"
+#include "src/core/lib/iomgr/iomgr_custom.h"
 #include "src/core/lib/iomgr/resolve_address.h"
 #include "src/core/lib/iomgr/timer.h"
 #include "src/core/lib/json/json.h"
@@ -430,8 +431,11 @@ static grpc_address_resolver_vtable ares_resolver = {
     grpc_resolve_address_ares, blocking_resolve_address_ares};
 
 static bool should_use_ares(const char* resolver_env) {
-  return resolver_env == nullptr || strlen(resolver_env) == 0 ||
+  // TODO(lidiz): Remove the "g_custom_iomgr_enabled" flag once c-ares support
-         gpr_stricmp(resolver_env, "ares") == 0;
+  // custom IO managers (e.g. gevent).
+  return !g_custom_iomgr_enabled &&
+         (resolver_env == nullptr || strlen(resolver_env) == 0 ||
+          gpr_stricmp(resolver_env, "ares") == 0);
 }
 
 void grpc_resolver_dns_ares_init() {

src/core/lib/iomgr/iomgr_custom.cc

@@ -49,6 +49,8 @@ static bool iomgr_platform_add_closure_to_background_poller(
   return false;
 }
 
+bool g_custom_iomgr_enabled = false;
+
 static grpc_iomgr_platform_vtable vtable = {
     iomgr_platform_init,
     iomgr_platform_flush,
@@ -61,6 +63,7 @@ void grpc_custom_iomgr_init(grpc_socket_vtable* socket,
                             grpc_custom_resolver_vtable* resolver,
                             grpc_custom_timer_vtable* timer,
                             grpc_custom_poller_vtable* poller) {
+  g_custom_iomgr_enabled = true;
   grpc_custom_endpoint_init(socket);
   grpc_custom_timer_init(timer);
   grpc_custom_pollset_init(poller);

src/core/lib/iomgr/iomgr_custom.h

@@ -39,6 +39,8 @@ extern gpr_thd_id g_init_thread;
 #define GRPC_CUSTOM_IOMGR_ASSERT_SAME_THREAD()
 #endif /* GRPC_CUSTOM_IOMGR_THREAD_CHECK */
 
+extern bool g_custom_iomgr_enabled;
+
 void grpc_custom_iomgr_init(grpc_socket_vtable* socket,
                             grpc_custom_resolver_vtable* resolver,
                             grpc_custom_timer_vtable* timer,

src/python/grpcio_tests/commands.py

@@ -153,6 +153,9 @@ class TestGevent(setuptools.Command):
         # TODO(https://github.com/grpc/grpc/issues/15411) enable this test
         'unit._cython._channel_test.ChannelTest.test_negative_deadline_connectivity'
     )
+    BANNED_WINDOWS_TESTS = (
+        # TODO(https://github.com/grpc/grpc/pull/15411) enable this test
+        'unit._dns_resolver_test.DNSResolverTest.test_connect_loopback',)
     description = 'run tests with gevent.  Assumes grpc/gevent are installed'
     user_options = []
 
@@ -178,6 +181,9 @@ class TestGevent(setuptools.Command):
         loader = tests.Loader()
         loader.loadTestsFromNames(['tests'])
         runner = tests.Runner()
+        if sys.platform == 'win32':
+            runner.skip_tests(self.BANNED_TESTS + self.BANNED_WINDOWS_TESTS)
+        else:
             runner.skip_tests(self.BANNED_TESTS)
         result = gevent.spawn(runner.run, loader.suite)
         result.join()

src/python/grpcio_tests/tests/tests.json

@@ -46,6 +46,7 @@
   "unit._cython.cygrpc_test.InsecureServerInsecureClient",
   "unit._cython.cygrpc_test.SecureServerSecureClient",
   "unit._cython.cygrpc_test.TypeSmokeTest",
+  "unit._dns_resolver_test.DNSResolverTest",
   "unit._empty_message_test.EmptyMessageTest",
   "unit._error_message_encoding_test.ErrorMessageEncodingTest",
   "unit._exit_test.ExitTest",

src/python/grpcio_tests/tests/unit/BUILD.bazel

@@ -14,6 +14,7 @@ GRPCIO_TESTS_UNIT = [
     "_channel_ready_future_test.py",
     "_compression_test.py",
     "_credentials_test.py",
+    "_dns_resolver_test.py",
     "_empty_message_test.py",
     "_exit_test.py",
     "_interceptor_test.py",

src/python/grpcio_tests/tests/unit/_dns_resolver_test.py

@@ -0,0 +1,63 @@
+# Copyright 2019 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.
+"""Tests for an actual dns resolution."""
+
+import unittest
+import logging
+import six
+
+import grpc
+from tests.unit import test_common
+from tests.unit.framework.common import test_constants
+
+_METHOD = '/ANY/METHOD'
+_REQUEST = b'\x00\x00\x00'
+_RESPONSE = _REQUEST
+
+
+class GenericHandler(grpc.GenericRpcHandler):
+
+    def service(self, unused_handler_details):
+        return grpc.unary_unary_rpc_method_handler(
+            lambda request, unused_context: request,
+        )
+
+
+class DNSResolverTest(unittest.TestCase):
+
+    def setUp(self):
+        self._server = test_common.test_server()
+        self._server.add_generic_rpc_handlers((GenericHandler(),))
+        self._port = self._server.add_insecure_port('[::]:0')
+        self._server.start()
+
+    def tearDown(self):
+        self._server.stop(None)
+
+    def test_connect_loopback(self):
+        # NOTE(https://github.com/grpc/grpc/issues/18422)
+        # In short, Gevent + C-Ares = Segfault. The C-Ares driver is not
+        # supported by custom io manager like "gevent" or "libuv".
+        with grpc.insecure_channel(
+                'loopback4.unittest.grpc.io:%d' % self._port) as channel:
+            self.assertEqual(
+                channel.unary_unary(_METHOD)(
+                    _REQUEST,
+                    timeout=test_constants.SHORT_TIMEOUT,
+                ), _RESPONSE)
+
+
+if __name__ == '__main__':
+    logging.basicConfig()
+    unittest.main(verbosity=2)