Skip to content

Conversation

@swathipil
Copy link
Member

@swathipil swathipil commented Sep 30, 2024

addresses: #37246

Creating the SSLContext and loading verify locations can be done in the constructor. Moving the check to the constructor so it doesn't need to be run in a ThreadPoolExecutor.

TODO:

  • test/confirm
    • test_sb_client_async.py:test_custom_endpoint_connection_verify_exception_async
    • test_negative_async.py:test_client_invalid_credential_async

@swathipil
Copy link
Member Author

/azp run python - eventhub - tests

@swathipil
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@swathipil swathipil changed the title [ServiceBus/EventHub] move async ssl opts to o transport constructor [ServiceBus/EventHub] move async ssl opts to transport constructor Sep 30, 2024
Copy link
Member

@l0lawrence l0lawrence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm :)

@swathipil swathipil marked this pull request as ready for review October 1, 2024 15:40
@swathipil swathipil requested a review from annatisch as a code owner October 1, 2024 15:40
@swathipil swathipil enabled auto-merge (squash) October 1, 2024 15:42
@sveinse
Copy link

sveinse commented Oct 1, 2024

Is it an acceptable solution to move blocking IO to the constructor of the class? A common use case of these objects is to dynamically create ServiceBusClient instances from coroutines which is under a running async event loop. Any blocking IO during initialization will restrict when and where in an async app these objects can be created, which I believe is limiting the usefulness of the fix. Thank you.

(I'm the reporter for the issue #37246)

@swathipil swathipil merged commit 295c432 into Azure:main Oct 1, 2024
@swathipil swathipil deleted the swathipil/eh/sslconfig-to-transport-constructor branch October 1, 2024 20:56
@swathipil
Copy link
Member Author

Hi @sveinse - We will discuss and let you know. Thanks.

@sveinse
Copy link

sveinse commented Oct 1, 2024

@swathipil thank you. If you're not aware of the comments in #37246 , please consider the new comments #37246 (comment) and onwards. Thanks.

w-javed pushed a commit to w-javed/azure-sdk-for-python that referenced this pull request Oct 3, 2024
…zure#37655)

* [ServiceBus/EventHub] move async ssl opts to o transport constructor

* changelog
l0lawrence pushed a commit to l0lawrence/azure-sdk-for-python that referenced this pull request Feb 19, 2025
…zure#37655)

* [ServiceBus/EventHub] move async ssl opts to o transport constructor

* changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants