-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[python] Avoid creating unused ThreadPools #1387
Merged
Merged
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,8 @@ class ApiClient(object): | |
the API. | ||
:param cookie: a cookie to include in the header when making calls | ||
to the API | ||
:param pool_threads: The number of threads to use for async requests | ||
to the API. More threads means more concurrent API requests. | ||
""" | ||
|
||
PRIMITIVE_TYPES = (float, bool, bytes, six.text_type) + six.integer_types | ||
|
@@ -53,14 +55,15 @@ class ApiClient(object): | |
'datetime': datetime.datetime, | ||
'object': object, | ||
} | ||
_pool = None | ||
|
||
def __init__(self, configuration=None, header_name=None, header_value=None, | ||
cookie=None): | ||
cookie=None, pool_threads=1): | ||
if configuration is None: | ||
configuration = Configuration() | ||
self.configuration = configuration | ||
self.pool_threads = pool_threads | ||
|
||
self.pool = ThreadPool() | ||
self.rest_client = rest.RESTClientObject(configuration) | ||
self.default_headers = {} | ||
if header_name is not None: | ||
|
@@ -70,8 +73,19 @@ class ApiClient(object): | |
self.user_agent = '{{#httpUserAgent}}{{{.}}}{{/httpUserAgent}}{{^httpUserAgent}}OpenAPI-Generator/{{{packageVersion}}}/python{{/httpUserAgent}}' | ||
|
||
def __del__(self): | ||
self.pool.close() | ||
self.pool.join() | ||
if self._pool: | ||
self._pool.close() | ||
self._pool.join() | ||
self._pool = None | ||
|
||
@property | ||
def pool(self): | ||
"""Create thread pool on first request | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
avoids instantiating unused threadpool for blocking clients. | ||
""" | ||
if self._pool is None: | ||
self._pool = ThreadPool(self.pool_threads) | ||
return self._pool | ||
|
||
@property | ||
def user_agent(self): | ||
|
2 changes: 1 addition & 1 deletion
2
samples/client/petstore/python-asyncio/.openapi-generator/VERSION
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
3.3.2-SNAPSHOT | ||
3.3.3-SNAPSHOT |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
samples/client/petstore/python-tornado/.openapi-generator/VERSION
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
3.3.2-SNAPSHOT | ||
3.3.3-SNAPSHOT |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a breaking change. I suggest leaving
=None
and still using default number of threads. It shouldn't be a problem because the pool will be created on the first request.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I'll set
pool_thread=None
cc @minrk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this is not a breaking change, though it is a change in the default performance of concurrent async requests from the same client object. Tying concurrent IO to CPU count doesn't make a lot of sense to me, especially in Python. I'd argue that a fixed default pool size (if not 1, maybe 4 or 10) would be more logical than spawning 64 threads on a big machine, and would result in more consistent behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@minrk I agree with you. This default may not work well on a huge server but it's default, it may be optimal in general, this calculation may be changed in the future releases of Python. Someone can base on this behavior and it'd be a breaking change for him/her.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using
pool_thread=None
for current master to make it backward-compatible and usingpool_threads=1
in 4.0.x (a major release with breaking changes scheduled to be released next month)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wing328 Sounds good to me.