Skip to content

Conversation

@meirdev
Copy link

@meirdev meirdev commented Oct 30, 2025

Summary

Currently AsyncClient creates a ThreadPoolExecutor in its constructor, I suggest allowing it to pass a ThreadPoolExecutor to solve two things:

  1. In case more than one AsyncClient is needed, there is no need to create a ThreadPoolExecutor for each connection.
  2. If the user called the run_in_executor function in his program, there is already a ThreadPoolExecutor that can be used and there is no need to create a new one.

Checklist

  • [ X] A human-readable description of the changes was provided to include in CHANGELOG

@CLAassistant
Copy link

CLAassistant commented Oct 30, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ joe-clickhouse
❌ Meir Elbaz


Meir Elbaz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@wiese-m
Copy link
Contributor

wiese-m commented Oct 31, 2025

Allowing passing executor_threads parameter along with executor parameter feels redundant to me. Maybe it would be a good idea to leave decision for the end user to just pass executor: Optional[Executor] = None where Executor is from concurrent.futures? And then in None case we set default behavior to create ThreadPoolExecutor() which already mimics max_workers parameter from standard library.

@meirdev
Copy link
Author

meirdev commented Nov 1, 2025

Yes, it would be better to omit the executor_threads parameter and use only executor, but that also breaks compatibility. So I leave it to the maintainers to decide.

Copy link
Contributor

@joe-clickhouse joe-clickhouse left a comment

Choose a reason for hiding this comment

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

Hi @meirdev, thanks! Overall looks good and will be useful for when users want to share a single ThreadPoolExecutor across multiple AsyncClient instances (e.g. to limit total resource usage) or when they want to use the app's existing executor for easier thread pool management. As for the PR, I think we'll just want to make the type hints more explicit and prevent allowing executor_threads to be None.

@joe-clickhouse
Copy link
Contributor

joe-clickhouse commented Nov 3, 2025

ok looks good! To merge this, we'll still need you to sign the CLA with your github email. Please see this comment. Thanks!

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Meir Elbaz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

As a side note, the whole typing thing revealed a bug that stems from the get_async_client default behavior which DOES allow None, but shouldn't. I have that tracked in #586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants