fix(socket-mode): only release connect_operation_lock in send_message when acquired - #1963
Open
dylanpulver wants to merge 1 commit into
Open
fix(socket-mode): only release connect_operation_lock in send_message when acquired#1963dylanpulver wants to merge 1 commit into
dylanpulver wants to merge 1 commit into
Conversation
… when acquired slackapi#1926 fixed this in AsyncBaseSocketModeClient.connect_to_new_endpoint, but the same `if self.connect_operation_lock.locked() is True: release()` shape is still in send_message() in both async backends, and neither had a test. websockets is the worse of the two: its retry block never acquires the lock at all, it only releases it. So whenever a reconnect holds the lock and a send_message() falls into the retry path, send_message() releases the reconnect's lock and two reconnects can run at once. The aiohttp sibling does acquire here, which is what makes the omission visible. aiohttp acquires but releases on locked(), so a task cancelled while waiting on acquire() releases whichever task does hold the lock. That is exactly the case slackapi#1926 describes. Both now track whether this task acquired, matching slackapi#1926 and the sync backends, which take the lock with `with` and cannot get this wrong.
|
Thanks for the contribution! Before we can merge this, we need @dylanpulver 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
#1926 fixed
if self.connect_operation_lock.locked() is True: release()inAsyncBaseSocketModeClient.connect_to_new_endpoint. The same shape is still insend_message()in both async backends.websocketsis the worse of the two: its retry block never acquires the lock, it only releases it infinally. So while a reconnect holds the lock, anysend_message()that falls into the retry path releases the reconnect's lock, and a second reconnect can start while the first is still running. Theaiohttpsibling does acquire here, which is what makes the omission visible.aiohttpacquires but releases based onlocked(), so a task cancelled while awaitingacquire()releases whichever task does hold the lock. That is the case #1926 describes.Both now track whether this task acquired. The three sync paths (
builtin,websocket_client,rtm_v2) take the lock withwithand cannot get this wrong, so they needed no change.Testing
A test per backend, in the style of the one added with #1926: another task holds the lock,
send_message()hits its retry path and is cancelled, and the lock must still be held afterwards. Each fails onmainand passes with the change.pytest tests/slack_sdk_async/socket_mode/— 22 passed (20 before these two)./scripts/lint.sh—ruff format --check455 files clean,ruff check slack/ slack_sdk/cleanmypy --config-file pyproject.tomlon both changed modules — no issuesI ran those individually rather than through
./scripts/run_validation.sh, so I have left that box unchecked.Category
/docs(Documents)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.