Skip to content
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

Add Socket Mode support to the SDK #879

Closed
wants to merge 8 commits into from
Closed

Conversation

seratch
Copy link
Member

@seratch seratch commented Nov 23, 2020

Summary

This pull request is still work in progress. We may change the APIs and/or implementations before releasing this.

This pull request adds all required functionalities for Socket Mode support, such as WSS URL retrieval, bidirectional communications with the Slack server over a WebSocket connection, maintaining WS connections, and relaying only envelope messages to Bolt framework or something similar.

In the initial version, the SDK is going to support the following major libraries for the low-level WebSocket client layer.

TODO:

The BaseSocketModeClient and AsyncBaseSocketModeClient provide good abstraction and the interface never depends on any of these specific library's interface. We accept the difference of a bit detailed configuration in the subtypes of the interface. For instance, websockets library does not support proxy settings at this moment. As aiohttp does not have OPEN message events, the implementation for this library does not offer on_open_listeners. These differences never appear in the base class (interface) level.

In this pull request, I have not implemented any decorators for envelope types (events_api, interactive, slash_commands) yet. If we receive developers' need to have such mechanism not only in Bolt for Python but also in slack_sdk.socket_mode module, we may consider adding the feature. But, in general, we highly recommend using Bolt for Python along with this SDK for complex app development.

related change: slackapi/bolt-python#154

Others I should mention are:

  • The GA release date is not yet publicly announced
  • Documentation is out of scope in this PR

Category (place an x in each of the [ ])

  • slack_sdk.socket_mode (New one!)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python setup.py validate after making the changes.

@seratch seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x oauth labels Nov 23, 2020
@seratch seratch self-assigned this Nov 23, 2020
@codecov
Copy link

codecov bot commented Nov 23, 2020

Codecov Report

Merging #879 (845fbab) into main (1c3878a) will decrease coverage by 11.47%.
The diff coverage is 0.17%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #879       +/-   ##
===========================================
- Coverage   76.28%   64.80%   -11.48%     
===========================================
  Files          35       44        +9     
  Lines        3226     3799      +573     
===========================================
+ Hits         2461     2462        +1     
- Misses        765     1337      +572     
Impacted Files Coverage Δ
slack_sdk/socket_mode/aiohttp/__init__.py 0.00% <0.00%> (ø)
slack_sdk/socket_mode/async_client.py 0.00% <0.00%> (ø)
slack_sdk/socket_mode/async_listeners.py 0.00% <0.00%> (ø)
slack_sdk/socket_mode/client.py 0.00% <0.00%> (ø)
slack_sdk/socket_mode/listeners.py 0.00% <0.00%> (ø)
slack_sdk/socket_mode/request.py 0.00% <0.00%> (ø)
slack_sdk/socket_mode/response.py 0.00% <0.00%> (ø)
slack_sdk/socket_mode/websocket_client/__init__.py 0.00% <0.00%> (ø)
slack_sdk/socket_mode/websockets/__init__.py 0.00% <0.00%> (ø)
slack_sdk/web/legacy_client.py 35.68% <33.33%> (-0.02%) ⬇️
... and 9 more

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 dc39395...f6a9cc7. Read the comment docs.

Copy link
Member Author

@seratch seratch left a comment

Choose a reason for hiding this comment

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

comments for reviewers @stevengill

Comment on lines 29 to 36
# TODO: remove this once Bolt allow skipping this
if self.app._signing_secret is None:
self.app._signing_secret = "just-for-workaround"
from slack_sdk.signature import SignatureVerifier

self.signature_verifier = SignatureVerifier(
signing_secret=self.app._signing_secret
)
Copy link
Member Author

Choose a reason for hiding this comment

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

these lines will be removed once slackapi/bolt-python#154 is shipped

listener = SocketModeAdapter(self.app).listener
self.client.socket_mode_request_listeners.append(listener)

async def connect_async(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

note: it's not technically possible to have asyncio.run here (in this class). so, the only thing this module can do for developers is provide async method this way.

async def close_async(self):
await self.client.close()

async def start_async(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

refer to the examples in this PR for usage

from slack_bolt.response import BoltResponse


class AsyncSocketModeAdapter:
Copy link
Member Author

Choose a reason for hiding this comment

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

don't worry, once we merge these changes into bolt-python library, I will commonize most of the code.

from slack_bolt.response import BoltResponse


class SocketModeAdapter:
Copy link
Member Author

Choose a reason for hiding this comment

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

eventually, this class will be placed under slack_bolt.adapter. that's why I named this as an adapter

app_token: str
wss_uri: Optional[str]
auto_reconnect_enabled: bool
web_socket_message_queue: Queue
Copy link
Member Author

Choose a reason for hiding this comment

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

I intentionally use web_socket_ and socket_mode_ prefix for field / method names to make the code clear but most of them look very long. We may want to remove web_socket_ as much as possible as it's obvious.

Comment on lines 94 to 96
_: Future[None] = asyncio.ensure_future(
self.run_web_socket_message_listeners(message, raw_message)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

not to process messages sequentially, using future.

if request is not None:
for listener in self.socket_mode_request_listeners:
try:
await listener(self, request)
Copy link
Member Author

Choose a reason for hiding this comment

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

the listener for Bolt is one of these listeners. you can add more listeners as necessary

Comment on lines +58 to +75
concurrency: int = 10,
trace_enabled: bool = False,
http_proxy_host: Optional[str] = None,
http_proxy_port: Optional[int] = None,
http_proxy_auth: Optional[Tuple[str, str]] = None,
proxy_type: Optional[str] = None,
on_open_listeners: Optional[List[Callable[[WebSocketApp], None]]] = None,
on_message_listeners: Optional[
List[Callable[[WebSocketApp, str], None]]
] = None,
on_error_listeners: Optional[
List[Callable[[WebSocketApp, Exception], None]]
] = None,
on_close_listeners: Optional[List[Callable[[WebSocketApp], None]]] = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

these are only available for websocket-client.

f"(error: {type(e).__name__}, message: {e})"
)

self.current_app_executor.submit(run_current_session)
Copy link
Member Author

Choose a reason for hiding this comment

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

not to block the current thread - WebSocketApp doesn't provide a way to establish WS connection without blocking the current thread.

@seratch
Copy link
Member Author

seratch commented Nov 24, 2020

I will be publishing 3.2.0a1 (pre release version) including this change for getting more feedback from Socket Mode beta testers. Let me know if other maintainers have concerns or suggestions.

@seratch
Copy link
Member Author

seratch commented Nov 25, 2020

published an alpha version (pre-release): https://pypi.org/project/slack-sdk/3.2.0a1/

@seratch seratch changed the title WIP: Add Socket Mode support to the SDK Add Socket Mode support to the SDK Nov 28, 2020
@seratch
Copy link
Member Author

seratch commented Dec 1, 2020

We will continue working on this task at #883

@seratch seratch closed this Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality oauth Version: 3x web-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant