-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
{Core} aaz
: Fix importlib deadlocks while loading requests.models
in multi threads
#26287
Conversation
️✔️AzureCLI-FullTest
|
Core |
@@ -12,6 +12,10 @@ | |||
from azure.core.polling.base_polling import LROBasePolling | |||
from azure.core.tracing.common import with_current_context | |||
from azure.core.tracing.decorator import distributed_trace | |||
# import LROPoller in main thread to resolve import deadlock between threads in python 3.10.10 | |||
# reference https://github.com/psf/requests/issues/2925 and https://github.com/Azure/azure-cli/issues/26272 | |||
from msrest.polling.poller import LROPoller # pylint: disable=unused-import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut feeling tells me this may not be safe enough, as there may be other dependencies imported by the sub-thread and main thread at the same time.
The SDK implementation is safer because the first PUT
operation is performed on the main thread, so subsequent polling GET
operations on the sub-thread will have the full dependency tree cached already.
Besides LROPoller
, there are also AzureOperationPoller
and AzureCoreLROPoller
imported from msrestazure.azure_operation
and azure.core.polling
respectively:
azure-cli/src/azure-cli-core/azure/cli/core/commands/__init__.py
Lines 1153 to 1157 in d9accde
def _is_poller(obj): | |
# Since loading msrest is expensive, we avoid it until we have to | |
if obj.__class__.__name__ in ['AzureOperationPoller', 'LROPoller', 'AAZLROPoller']: | |
return isinstance(obj, poller_classes()) | |
return False |
azure-cli/src/azure-cli-core/azure/cli/core/util.py
Lines 685 to 690 in dae19d0
def poller_classes(): | |
from msrestazure.azure_operation import AzureOperationPoller | |
from msrest.polling.poller import LROPoller | |
from azure.core.polling import LROPoller as AzureCoreLROPoller | |
from azure.cli.core.aaz._poller import AAZLROPoller | |
return (AzureOperationPoller, LROPoller, AzureCoreLROPoller, AAZLROPoller) |
Though I haven't got time to look into their actual usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is enough to mitigate the issue. Moving the first request in the main thread will involve a huge change.
We cannot resolve them all because it's python issue python/cpython#91351.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When loading other modules, some of them will import requests
. That's why it can mitigate the issue.
… in multi threads (Azure#26287) * import LROPoller in main thread to resolve importlib deadlock in python 3.10.10 * use 'import requests' * fix style issue * update comments
… in multi threads (Azure#26287) * import LROPoller in main thread to resolve importlib deadlock in python 3.10.10 * use 'import requests' * fix style issue * update comments
closed: #26272
closed: #26297
Related command
Description
In python 3.10.10, we encountered import deadlocks in
aaz
LRO which try to load therequests.models
in two different threads. It's a known issue psf/requests#2925In this PR, we provide a workaround by importing these modules in the main thread before spawning child threads.
Testing Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a
: Make some customer-facing breaking change[Component Name 2]
az command b
: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.