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.

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

Jump to bottom

Web client and rtm client should be fully split, and web client should not use asyncio #633

Closed
5 of 9 tasks
ryan-lane opened this issue Mar 5, 2020 · 4 comments
Closed
5 of 9 tasks

Web client and rtm client should be fully split, and web client should not use asyncio #633

ryan-lane opened this issue Mar 5, 2020 · 4 comments
Assignees
@seratch
Labels
area:concurrency Issues and PRs related to concurrency rtm-client Version: 2x Version: 3x web-client
Milestone
3.0.0

Comments

@ryan-lane
Copy link

Description

The 2.x release of slackclient is completely unusable in projects not based on asyncio. The integration of asyncio is too deep into the library. Whether to use async or not in api calls should be up to the caller, not baked into the library. I've spent the last two weeks trying to upgrade my project based only on the web APIs, and so many things are broken that I believe I'm going to have to stay on 1.x forever, or look for a new library unless the asyncio code is removed.

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • testing related
  • discussion

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

  • 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 searched for any related issues and avoided creating a duplicate issue.

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Reproducible in:

slackclient version: 2.x

python version: 3.6+
@stevengill stevengill added the area:concurrency Issues and PRs related to concurrency label Mar 5, 2020
@stevengill
Copy link
Member

Hey @ryan-lane,

Sorry for the trouble. We are aware of various issues with the 2.x python client. You can view the tag area:concurrency in our issue tracker to see the various problems with the existing approach.

We have a plan in place to release a 3.x version soon. We will most likely be splitting the web client into two. One for sync operations and one for async (asyncio based). For the async one, we also plan leaving managing the event loop to the library users instead of the library trying to do that for you.

@ryan-lane
Copy link
Author

@stevengill that's great to know. Thanks! I'll try again with 3.x later

@seratch seratch modified the milestones: 3.x, 2.6.0 Apr 20, 2020
seratch added a commit to seratch/python-slack-sdk that referenced this issue Apr 27, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
6675858 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
@seratch seratch mentioned this issue Apr 27, 2020
Merged
2 tasks
seratch added a commit to seratch/python-slack-sdk that referenced this issue Apr 28, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
613f3f7 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue Apr 28, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
6f642bb 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue Apr 30, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
fb21a25 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 11, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
618a6ba 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 11, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
3b55024 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 11, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
af57ecd 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 11, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
e6792db 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 11, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
0a172b3 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 11, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
5a1b59b 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 11, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
3821499 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 13, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
ae3391e 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
@seratch
Copy link
Member

seratch commented May 14, 2020

This would be partially resolved by #662

With the changes in the PR, WebClient w/ run_async=False (the default mode) no longer depends on asyncio and aiohttp in terms of its behaviors. By contrast, RTMClient is not free from aiohttp as of version 2.6.x. For the future, I've not yet decided if I should use my time to get rid of asyncio/aiohttp in RTMClient (I'm currently working on the Bolt for Python project which has more priority).

From here, this is my personal opinion at this point.

In version 3.x, WebClient can be more explicit about the difference between async and sync mode. For instance, currently, the API methods in WebClient are not async methods (in other words, coroutines) even if its run_async is True. If those methods were async ones and this library provides ways to choose any other non-blocking HTTP clients, this library can be more usable in more various use cases. In particular, it may be crucial for providing async-friendly Bolt for Python. I would love to hear ideas from the community regarding this.

@seratch seratch modified the milestones: 2.6.0, 3.0.0 May 14, 2020
seratch added a commit to seratch/python-slack-sdk that referenced this issue May 14, 2020
@seratch
Revise WebClient/RTMClient internals for run_async=False
b86a8fc 
* slackapi#530 Fixed by changing _execute_in_thread to be a coroutine
* slackapi#569 Resolved by removing a blocking loop (while future.running())
* slackapi#645 WebClient(run_async=False) no longer depends on asyncio by default
* slackapi#633 WebClient(run_async=False) doesn't internally depend on aiohttp
* slackapi#631 When run_async=True, RTM listner can be a normal function and WebClient is free from the event loop
* slackapi#630 WebClient no longer depends on aiohttp when run_async=False
* slackapi#497 Fixed when run_async=False / can be closed as we don't support run_async=True for this use case (in Flask)
@seratch seratch modified the milestones: 3.1.0, 3.0.0 Jul 29, 2020
This was referenced Jul 29, 2020
Closed
Merged
@seratch
Copy link
Member

seratch commented Oct 27, 2020

v3.0.0 no longer has required dependencies and the internals of WebClient/WebhookClient doesn't depend on AIOHTTP at all. You can safely use the API client without dealing with asyncio specific matters.

Now that v3.0.0rc1 is out, let me close this issue. If you have feedback on the v3 package, please let us know by writing in here or creating a new issue for it.

@seratch seratch closed this as completed Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@seratch seratch

Labels
area:concurrency Issues and PRs related to concurrency rtm-client Version: 2x Version: 3x web-client
Projects
None yet
Milestone
3.0.0
Development

No branches or pull requests
3 participants
@seratch @stevengill @ryan-lane