Repository navigation
fix(socket_mode): recreate a closed aiohttp session when reconnecting #1966
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
Changes from 1 commit
4a754cd
89b7542
f193467
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -127,6 +127,7 @@ def __init__( | |||||||||||||||||
| # over the lifetime of your application, | ||||||||||||||||||
| # it is suggested you use a single session for the lifetime of your application | ||||||||||||||||||
| # to benefit from connection pooling. | ||||||||||||||||||
| self._loop = loop | ||||||||||||||||||
| self.aiohttp_client_session = aiohttp.ClientSession(loop=loop) | ||||||||||||||||||
|
|
||||||||||||||||||
| self.on_message_listeners = on_message_listeners or [] | ||||||||||||||||||
|
|
@@ -322,6 +323,7 @@ async def is_connected(self) -> bool: | |||||||||||||||||
| connected: bool = ( | ||||||||||||||||||
| not self.closed | ||||||||||||||||||
| and not self.stale | ||||||||||||||||||
| and not self.aiohttp_client_session.closed | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this clause can go. |
||||||||||||||||||
| and self.current_session is not None | ||||||||||||||||||
| and not self.current_session.closed | ||||||||||||||||||
| and not await self.is_ping_pong_failing() | ||||||||||||||||||
|
|
@@ -369,6 +371,12 @@ async def connect(self): | |||||||||||||||||
| except Exception as e: | ||||||||||||||||||
| self.logger.exception(f"Failed to close the old session : {e}") | ||||||||||||||||||
|
|
||||||||||||||||||
| if self.aiohttp_client_session.closed: | ||||||||||||||||||
| # All connections share this session. Once it has been closed, | ||||||||||||||||||
| # every connection attempt fails, so it has to be recreated. | ||||||||||||||||||
| self.logger.info("The aiohttp client session is closed; creating a new one") | ||||||||||||||||||
| self.aiohttp_client_session = aiohttp.ClientSession(loop=self._loop) | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be simplified, with one extra guard?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed all three suggestions in 89b7542: removed the stored loop, create the replacement session with no arguments and the extra shutdown guard, and removed the HTTP-session clause from |
||||||||||||||||||
|
|
||||||||||||||||||
| if self.wss_uri is None: | ||||||||||||||||||
| # If the underlying WSS URL does not exist, | ||||||||||||||||||
| # acquiring a new active WSS URL from the server-side first | ||||||||||||||||||
|
|
||||||||||||||||||
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.
I don't think we need to store the loop.
connect()always runs inside the running event loop, andaiohttp.ClientSession()binds to the running loop by default. So we can drop this attribute and create the session with no arguments below.