Skip to content

Commit

Permalink
[Fix Python Deadlock] Guard grpc_ssl_credentials_create with nogil (g…
Browse files Browse the repository at this point in the history
…rpc#34712)

Fix: grpc#34672
With some recent changes in core, now `grpc_ssl_credentials_create` is
guarded by `gpr_once_init`. In our current implementation, The thread
got `gpr_once_init` lock might require GIL lock during the execution of
`grpc_ssl_credentials_create`, which might cause a deadlock if another
thread is holding GIL lock and waiting for `gpr_once_init` lock.

This change adds `with nogil` to calls to native function
`grpc_ssl_credentials_create` to make sure GIL is released before
calling `grpc_ssl_credentials_create`.
<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->
  • Loading branch information
XuanWang-Amos authored Oct 18, 2023
1 parent 301636d commit ccfc5b9
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 4 deletions.
10 changes: 6 additions & 4 deletions src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,9 @@ cdef class SSLChannelCredentials(ChannelCredentials):
else:
c_pem_root_certificates = self._pem_root_certificates
if self._private_key is None and self._certificate_chain is None:
return grpc_ssl_credentials_create(
c_pem_root_certificates, NULL, NULL, NULL)
with nogil:
return grpc_ssl_credentials_create(
c_pem_root_certificates, NULL, NULL, NULL)
else:
if self._private_key:
c_pem_key_certificate_pair.private_key = self._private_key
Expand All @@ -164,8 +165,9 @@ cdef class SSLChannelCredentials(ChannelCredentials):
c_pem_key_certificate_pair.certificate_chain = self._certificate_chain
else:
c_pem_key_certificate_pair.certificate_chain = NULL
return grpc_ssl_credentials_create(
c_pem_root_certificates, &c_pem_key_certificate_pair, NULL, NULL)
with nogil:
return grpc_ssl_credentials_create(
c_pem_root_certificates, &c_pem_key_certificate_pair, NULL, NULL)


cdef class CompositeChannelCredentials(ChannelCredentials):
Expand Down
23 changes: 23 additions & 0 deletions src/python/grpcio_tests/tests/unit/_api_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@
"""Test of gRPC Python's application-layer API."""

import logging
import threading
import unittest

import grpc

from tests.unit import _from_grpc_import_star
from tests.unit import test_common


class AllTest(unittest.TestCase):
Expand Down Expand Up @@ -115,6 +117,27 @@ def test_secure_channel(self):
channel = grpc.secure_channel("google.com:443", channel_credentials)
channel.close()

def test_multiple_secure_channel(self):
_THREAD_COUNT = 10
wait_group = test_common.WaitGroup(_THREAD_COUNT)

def create_secure_channel():
channel_credentials = grpc.ssl_channel_credentials()
wait_group.done()
wait_group.wait()
channel = grpc.secure_channel("google.com:443", channel_credentials)
channel.close()

threads = []
for _ in range(_THREAD_COUNT):
thread = threading.Thread(target=create_secure_channel)
thread.setDaemon(True)
thread.start()
threads.append(thread)

for thread in threads:
thread.join()


if __name__ == "__main__":
logging.basicConfig()
Expand Down

0 comments on commit ccfc5b9

Please sign in to comment.