Skip to content

Handle case in which aiohttp closes the websocket due to lack of ping responses. #453

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.

Sign up for GitHub

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

Merged
RodneyU215 merged 2 commits into from
Jun 29, 2019
Merged

Handle case in which aiohttp closes the websocket due to lack of ping responses. #453

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36c28ce
Handle case in which aiohttp closes the websocket due to lack of ping…
flyte Jun 21, 2019
76b0d13
Dispatch error event when error received over websocket
flyte Jun 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 48 additions & 12 deletions slack/rtm/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,14 @@ async def _connect_and_read(self):
self._websocket = websocket
await self._dispatch_event(event="open", data=data)
await self._read_messages()
# The websocket has been disconnected, or self._stopped is True
if not self._stopped and not self.auto_reconnect:
self._logger.warning(
"Not reconnecting the Websocket because auto_reconnect is False"
)
return
# No need to wait exponentially here, since the connection was
# established OK, but timed out, or was closed remotely
except (
client_err.SlackClientNotConnectedError,
client_err.SlackApiError,
Expand All @@ -351,18 +359,45 @@ async def _connect_and_read(self):
raise

async def _read_messages(self):
"""Process messages received on the WebSocket connection.

Iteration terminates when the client is stopped or it disconnects.
"""
"""Process messages received on the WebSocket connection."""
while not self._stopped and self._websocket is not None:
async for message in self._websocket:
if message.type == aiohttp.WSMsgType.TEXT:
payload = message.json()
event = payload.pop("type", "Unknown")
await self._dispatch_event(event, data=payload)
elif message.type == aiohttp.WSMsgType.ERROR:
break
try:
# Wait for a message to be recieved, but timeout after a second so that
# we can check if the socket has been closed, or if self._stopped is
# True
message = await self._websocket.receive(timeout=1)
except asyncio.TimeoutError:
if not self._websocket.closed:
Copy link
Contributor

@RodneyU215 RodneyU215 Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see some unit tests added for this condition.

Copy link
Contributor Author

@flyte flyte Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've spent some time trying to mock up enough to get this tested but I've come up short I'm afraid, and I need to get on with some work. Do you have any bright ideas about how to test this section?

# We didn't receive a message within the timeout interval, but
# aiohttp hasn't closed the socket, so ping responses must still be
# returning
continue
self._logger.warning(
"Websocket was closed (%s).", self._websocket.close_code
)
await self._dispatch_event(
event="error", data=self._websocket.exception()
)
self._websocket = None
await self._dispatch_event(event="close")
return
if message.type == aiohttp.WSMsgType.TEXT:
payload = message.json()
event = payload.pop("type", "Unknown")
await self._dispatch_event(event, data=payload)
elif message.type == aiohttp.WSMsgType.ERROR:
self._logger.error("Received an error on the websocket: %r", message)
await self._dispatch_event(event="error", data=message)
elif message.type in (
aiohttp.WSMsgType.CLOSE,
aiohttp.WSMsgType.CLOSING,
aiohttp.WSMsgType.CLOSED,
):
self._logger.warning("Websocket was closed.")
self._websocket = None
await self._dispatch_event(event="close")
else:
self._logger.debug("Received unhandled message type: %r", message)

async def _dispatch_event(self, event, data=None):
"""Dispatches the event and executes any associated callbacks.
Expand Down Expand Up @@ -390,7 +425,8 @@ async def _dispatch_event(self, event, data=None):
)
try:
if self._stopped and event not in ["close", "error"]:
# Don't run callbacks if client was stopped unless they're close/error callbacks.
# Don't run callbacks if client was stopped unless they're
# close/error callbacks.
break

if inspect.iscoroutinefunction(callback):
Expand Down