New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
asyncio-based Socket Mode client improvements #1117
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.
|
async def is_connected(self) -> bool: | ||
return ( | ||
not self.closed | ||
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the ping-pong validation logic to is_ping_pong_failing method and added it to is_connected() method
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. |
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_time
with 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_session
directlyFor both AIOHTTP and websockets implementations, this pull request improves the internals of
monitor_current_session
andreceive_messages
not 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
x
in 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
x
in each[ ]
)python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh
after making the changes.