Skip to content

[Fix Python Deadlock] Guard grpc_ssl_credentials_create with nogil#34712

Merged
XuanWang-Amos merged 2 commits intogrpc:masterfrom
XuanWang-Amos:fix_ssl_create_hanging
Oct 18, 2023
Merged

[Fix Python Deadlock] Guard grpc_ssl_credentials_create with nogil#34712
XuanWang-Amos merged 2 commits intogrpc:masterfrom
XuanWang-Amos:fix_ssl_create_hanging

Conversation

@XuanWang-Amos
Copy link
Copy Markdown
Contributor

@XuanWang-Amos XuanWang-Amos commented Oct 17, 2023

Fix: #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.

@XuanWang-Amos XuanWang-Amos added the release notes: no Indicates if PR should not be in release notes label Oct 17, 2023
@XuanWang-Amos XuanWang-Amos changed the title [Fix Python deadlock] Guard grpc_ssl_credentials_create with nogil [Fix Python Deadlock] Guard grpc_ssl_credentials_create with nogil Oct 17, 2023
@XuanWang-Amos XuanWang-Amos requested a review from gnossen October 17, 2023 21:18
@XuanWang-Amos XuanWang-Amos marked this pull request as ready for review October 17, 2023 22:00
@XuanWang-Amos XuanWang-Amos merged commit ccfc5b9 into grpc:master Oct 18, 2023
@XuanWang-Amos XuanWang-Amos deleted the fix_ssl_create_hanging branch October 18, 2023 16:40
XuanWang-Amos added a commit to XuanWang-Amos/grpc that referenced this pull request Oct 18, 2023
…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.

-->
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Oct 18, 2023
XuanWang-Amos added a commit that referenced this pull request Oct 18, 2023
…1.59.x backport) (#34725)

Backport of #34712 to v1.59.x.
---
Fix: #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.

-->
gtcooke94 added a commit that referenced this pull request Oct 18, 2023
#34726)

This reverts commit 601aaf8, which
results in rolling forward #34408.

The 601aaf reversion happened because of a deadlock found in Python. The
root cause ended up being an issue with the Python wrapper and was fixed
in #34712 , so this can be rolled forward again
copybara-service bot pushed a commit that referenced this pull request Apr 9, 2024
…th nogil (#36266)

This fix is similar to #34712 but for `grpc_google_default_credentials_create` rather than `grpc_ssl_credentials_create`

Fixes #36265
Fixes googleapis/python-bigtable#949

Closes #36266

COPYBARA_INTEGRATE_REVIEW=#36266 from parthea:repro-issue-34672 d736f6f
PiperOrigin-RevId: 623291826
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request Apr 16, 2024
…th nogil (grpc#36266)

This fix is similar to grpc#34712 but for `grpc_google_default_credentials_create` rather than `grpc_ssl_credentials_create`

Fixes grpc#36265
Fixes googleapis/python-bigtable#949

Closes grpc#36266

COPYBARA_INTEGRATE_REVIEW=grpc#36266 from parthea:repro-issue-34672 d736f6f
PiperOrigin-RevId: 623291826
XuanWang-Amos added a commit that referenced this pull request Apr 16, 2024
…th nogil (v1.62.x backport) (#36376)

Backport of #36266 to v1.62.x.
---
This fix is similar to #34712 but for
`grpc_google_default_credentials_create` rather than
`grpc_ssl_credentials_create`

Fixes #36265
Fixes googleapis/python-bigtable#949

Co-authored-by: Anthonios Partheniou <partheniou@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

imported Specifies if the PR has been imported to the internal repository lang/Python release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(python): Possible regression in grpcio==1.59.0 related to grpc thread pool hanging

2 participants