asyncio-based Socket Mode client improvements#1117
Merged
seratch merged 5 commits intoslackapi:mainfrom Sep 18, 2021
Merged
Conversation
…-based SocketModeClient
Codecov Report
@@ Coverage Diff @@
## main #1117 +/- ##
==========================================
+ Coverage 84.96% 88.08% +3.11%
==========================================
Files 110 110
Lines 10572 10681 +109
==========================================
+ Hits 8983 9408 +425
+ Misses 1589 1273 -316
Continue to review full report at Codecov.
|
seratch
commented
Sep 17, 2021
| and not self.stale | ||
| and self.current_session is not None | ||
| and not self.current_session.closed | ||
| and not await self.is_ping_pong_failing() |
Contributor
Author
There was a problem hiding this comment.
Moved the ping-pong validation logic to is_ping_pong_failing method and added it to is_connected() method
Contributor
Author
|
I've done testing with these changes and found that the improvements here can eliminate (at least mitigate) some race conditions / invalid data references while reconnection. Let me merge this and release a new patch version including the changes. |
16 tasks
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
1 participant
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
This pull request applies a few improvements to asyncio-based Socket Mode clients.
connect()method in AIOHTTP based clientThis pull request adds a missing change in the pull request #1112
Like the built-in client implementation does, sending our own ping message to check if the connection is healthy can make the session check logic more accurate. If the connection is working, the client instance receives a new PONG message immediately and update the
self.last_ping_pong_timewith the timestamp of the latest trial.This change can improve the situation described in this comment: #1110 (comment) (= the situation where the current connection is determined as stale 10 seconds after the last reconnection).
self.current_sessiondirectlyFor both AIOHTTP and websockets implementations, this pull request improves the internals of
monitor_current_sessionandreceive_messagesnot to access the instance fieldself.current_session. Instead of that, now these methods accesses only the local variable set when the method execution starts. While doing more tests with these clients, I found that accessing shared objects from tasks can cause unexpected race conditions and invalid data reference. The changes in this PR will make the executions of these methods much safer.Category (place an
xin each of the[ ])/docs-src(Documents, have you run./scripts/docs.sh?)/docs-src-v2(Documents, have you run./scripts/docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.