Skip to content

Fix #558 #619 by revising RTMClient to accept simultaneous incoming messages#667

Merged
seratch merged 1 commit intoslackapi:masterfrom
seratch:issue-558
May 11, 2020
Merged

Fix #558 #619 by revising RTMClient to accept simultaneous incoming messages#667
seratch merged 1 commit intoslackapi:masterfrom
seratch:issue-558

Conversation

@seratch
Copy link
Contributor

@seratch seratch commented May 1, 2020

Summary

This pull request fixes #558 #619.

The cause of the issue addressed by this pull request is that the current implementation of RTMClient handles incoming WebSocket messages one by one (=sequentially). As a result, the following code blocks other handlers until the sleep time (10 seconds) passes.

@RTMClient.run_on(event="message")
async def process_messages(**payload):
    self.logger.debug(payload)
    await asyncio.sleep(10)  # this used to block all other handlers

This pull request changes the internals of RTMClient a bit to accept simultaneous text messages. Error events and connection close events should not be executed asynchronously.

@stevengill @aoberoi @shaydewael
As #665 and this pull request have a conflict, I need to merge them into one. But I think having reviews separately should be easier for these cases. After waiting for reviews by others for a few business days, I'll come up with a unified pull request and close this and #665.

Requirements (place an x in each [ ])

@seratch seratch added Version: 2x bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented rtm-client v2.6 in-progress labels May 1, 2020
@seratch seratch added this to the 2.6.0 milestone May 1, 2020
@seratch seratch self-assigned this May 1, 2020
@codecov
Copy link

codecov bot commented May 1, 2020 •
edited
Loading

Codecov Report

Merging #667 into master will increase coverage by 0.00%.
The diff coverage is 54.54%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #667   +/-   ##
=======================================
  Coverage   87.17%   87.17%           
=======================================
  Files          15       15           
  Lines        1824     1833    +9     
  Branches      100      103    +3     
=======================================
+ Hits         1590     1598    +8     
+ Misses        202      201    -1     
- Partials       32       34    +2
Impacted Files Coverage Δ
slack/rtm/client.py 84.61% <54.54%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fb34e9...5f37f66. Read the comment docs.

stevengill
stevengill approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work!

slack/rtm/client.py
if num_of_running_callbacks > 0:
self._logger.info(
"WebSocket connection has been closed "
f"though {num_of_running_callbacks} callback executions were still in progress"
Copy link
Member

@stevengill stevengill May 7, 2020

Choose a reason for hiding this comment

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

This is just a msg for the devs that they lost some executions by running close? Guessing we wouldn't want to halt the close event

Copy link
Contributor Author

@seratch seratch May 7, 2020

Choose a reason for hiding this comment

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

Yes, as you guessed, this message is meant for giving a bit more information for developers for the case they unexpectedly encounter this situation. This message indicates that if some of the remaining tasks are going to send messages back to the Slack server over a WebSocket connection, it won't work. If the remaining tasks don't talk to the Slack server, they're not terminated yet, so that they may result in success.

@seratch seratch merged commit 7fe225f into slackapi:master May 11, 2020
@seratch seratch deleted the issue-558 branch May 11, 2020 01:18
@seratch seratch mentioned this pull request May 14, 2020
Closed
9 tasks
This was referenced May 25, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevengill stevengill stevengill approved these changes

@aoberoi aoberoi Awaiting requested review from aoberoi

@shaydewael shaydewael Awaiting requested review from shaydewael

Assignees

@seratch seratch

Labels

bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented rtm-client Version: 2x

Projects

None yet

Milestone

2.6.0

Development

Successfully merging this pull request may close these issues.

Unexpected behavior in that dispatched events are not asynchronous

2 participants

@seratch @stevengill