[ServiceBus] Fix async sender AttributeError race in _open (#35618) - #46683
[ServiceBus] Fix async sender AttributeError race in _open (#35618)#46683Sai Asish Y (SAY-5) wants to merge 2 commits into
Conversation
Capture self._handler into a local before awaiting open_async/client_ready_async so a concurrent coroutine cannot null it out mid-flight, addressing the AttributeError reported in Azure#35618. Signed-off-by: SAY-5 <saiasish.cnp@gmail.com>
|
Thank you for your contribution Sai Asish Y (@SAY-5)! We will review the pull request and get back to you soon. |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a concurrency bug in the async Service Bus sender by making _open() use a local handler reference, so shared ServiceBusSender instances are less likely to hit an AttributeError during concurrent sends. It fits into the azure-servicebus package's async connection lifecycle, specifically around sender startup and readiness checks.
Changes:
- Updated
ServiceBusSender._open()to captureself._handlerin a local variable before awaited startup calls. - Switched handler-dependent readiness and max-message-size logic in
_open()to use the captured local handler. - Added a changelog entry describing the async sender race-condition fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
sdk/servicebus/azure-servicebus/azure/servicebus/aio/_servicebus_sender_async.py |
Adjusts async sender open logic to use a local handler reference during awaited startup. |
sdk/servicebus/azure-servicebus/CHANGELOG.md |
Documents the intended bug fix for the upcoming unreleased version. |
| handler = self._handler | ||
| try: | ||
| await self._handler.open_async(connection=self._connection) | ||
| while not await self._handler.client_ready_async(): | ||
| await handler.open_async(connection=self._connection) | ||
| while not await handler.client_ready_async(): | ||
| await asyncio.sleep(0.05) | ||
| self._running = True | ||
| self._max_message_size_on_link = ( | ||
| self._amqp_transport.get_remote_max_message_size(self._handler) or MAX_MESSAGE_LENGTH_BYTES | ||
| self._amqp_transport.get_remote_max_message_size(handler) or MAX_MESSAGE_LENGTH_BYTES |
| try: | ||
| await self._handler.open_async(connection=self._connection) | ||
| while not await self._handler.client_ready_async(): | ||
| await handler.open_async(connection=self._connection) |
Capture critical section in a per-sender lock so concurrent coroutines cannot race on creating and closing self._handler. Re-checks _running inside the lock so the loser of the race no-ops cleanly. Signed-off-by: SAY-5 <say.apm35@gmail.com>
|
Addressed the serialization concern in f568e44: wrapped the create/open critical section in a per-sender I left out a regression test in this commit, the package's existing test infra runs against live Service Bus (test_proxy fixture, no async sender mock harness today). Happy to add a unit test alongside if you'd like, but it would mean introducing a mock sender pattern that doesn't yet exist here, which felt like a separate change to keep this fix reviewable. Let me know your preference. |
|
Heads-up: the Azure DevOps Build Test (Ubuntu2404_313) and Build Analyze are red on f568e44 but I can't see the pipeline log without an azure-sdk org account. Local |
|
Reproduced the Build Analyze pyright error locally, the reportIncompatibleMethodOverride on _open is pre-existing on main as well, not introduced by this PR. Happy to follow up with a separate annotation fix if desired. |
f568e44 to
bb2f8d6
Compare
|
Hi Sai Asish Y (@SAY-5). Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
|
Still active, the fix and the pyright discussion above stand as is unless there's further feedback. |
Description
Fixes #35618.
When multiple coroutines concurrently call
ServiceBusSender.send_messageson a shared async sender, a race in_servicebus_sender_async.py:_opencan null outself._handlerbetweenawait self._handler.open_async(...)andawait self._handler.client_ready_async(), producing:This patch captures
self._handlerinto a local variable immediately after_create_handler(auth)and uses that local for the subsequent awaits within_open. Theexcept/_close_handlercleanup path still operates onself._handler, preserving existing teardown semantics.All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines