Only release connect_operation_lock when this task acquired it - #1926
Open
ckarnell wants to merge 1 commit into
Open
Only release connect_operation_lock when this task acquired it#1926ckarnell wants to merge 1 commit into
ckarnell wants to merge 1 commit into
Conversation
AsyncBaseSocketModeClient.connect_to_new_endpoint() releases the lock whenever connect_operation_lock.locked() is true, rather than when this coroutine actually acquired it. asyncio.Lock has no notion of ownership, so release() from a task that never held the lock succeeds and frees it for everyone. That is reachable on an ordinary reconnect. connect() cancels message_receiver and current_session_monitor on every successful reconnection, and both of those tasks call connect_to_new_endpoint(). So one of them can be cancelled while suspended inside acquire(), and its finally block then releases the lock belonging to the reconnect still in progress, dropping mutual exclusion around it. Track acquisition in a local instead, which is what the synchronous BaseSocketModeClient already does with its acquired flag.
|
Thanks for the contribution! Before we can merge this, we need @ckarnell to sign the Salesforce Inc. Contributor License Agreement. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AsyncBaseSocketModeClient.connect_to_new_endpoint()releasesconnect_operation_lockwhenever the lock is merelylocked(), rather than when this coroutine actually acquired it:asyncio.Lockhas no notion of ownership, unlikethreading.Lock. Arelease()call from a task that never held the lock succeeds and frees it for everyone.Why this is reachable on an ordinary reconnect, not just at shutdown
connect()cancels both background tasks on every successful reconnection:and both of those tasks call
connect_to_new_endpoint()(aiohttp/__init__.py, inmonitor_current_session()andreceive_messages()). So:monitor_current_sessioncallsconnect_to_new_endpoint(), acquires the lock, and is insideawait self.connect().receive_messagesreaches its own reconnect path and suspends insideconnect_operation_lock.acquire().connect(),self.message_receiver.cancel()cancels the second task while it is waiting on the lock.CancelledErrorpropagates out ofacquire(), so that task never acquired anything, butlocked()isTruebecause the first task still holds it.finallyblock releases the first task's lock, mid-reconnect.Mutual exclusion around the reconnect is gone, so another reconnect can start while one is still in flight.
Reproduction
tests/slack_sdk_async/socket_mode/test_async_client_lock.pydrives the realAsyncBaseSocketModeClientwith aconnect()that blocks until released, cancels a second caller while it waits on the lock, and asserts the lock is still held.Without the change:
With the change it passes.
The fix
Track acquisition in a local, which is exactly what the synchronous
BaseSocketModeClientalready does:So this makes the async client consistent with its sync counterpart. Three lines, no behaviour change on the happy path: when the coroutine does acquire the lock,
acquiredisTrueand it releases exactly as before.What I verified
tests/slack_sdk_async/socket_mode/passes: 14 passed.black --line-length 125(the setting inpyproject.toml) leaves both files unchanged.main, not only in the released package.What I did not check
Whether this is the cause of any specific reported disconnection. The area has prior history (#1110, #1112) whose symptoms are consistent with losing mutual exclusion around reconnects, but I have not tied this to a particular report, and I have not tried to reproduce it against live Slack infrastructure.
A note on severity
Not a security issue, and it needs a cancellation to land in a narrow window, so it is unlikely to be a frequent cause of trouble. The consequence when it does happen is two concurrent reconnects rather than data loss. The sync client already gets this right, which is the main argument for the change.
Category
slack_sdk.socket_mode(Socket Mode client)Requirements
./scripts/run_validation.shafter making the changes. Not run in full. What I did run is listed under "What I verified" above: thetests/slack_sdk_async/socket_mode/suite (14 passed) andblack --line-length 125on both changed files. Happy to run the full script if you would like it before review.