-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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] Cleanup ThreadPool with atexit rather than __del__ #5094
Conversation
👍 Thanks for opening this issue! The team will review the labels and make any necessary changes. |
Thank you for this PR. Can you add a test verifying that the garbage collection is cleaning up the threadpool? |
@spacether hmm, probably, though I'm not totally sure how off the top of my head. I'll dig into it a bit, did you have an approach in mind? |
Perhaps use gc.collect per here https://stackoverflow.com/questions/1641717/manual-garbage-collection-in-python |
No, please don't. This is not supported in CPython. It's a programming paradigm that might work in other languages, but not CPython. The whole point of this PR is that thread pools should not be handled within a garbage collector as that puts you in undefined space. Remember in Python it's not guaranteed the garbage collector will ever be called. If the user decided that they don't manually close down the threadpool the only fallback is to do it via atexit as the final line of defence. In case you have resources though it's recommended to encourage usage of the objects via the |
Thank you for letting us know that using the gc library is the wrong way to test this. @gaborbernat/@fabianvf what do you think of us also adding
@fabianvf
Can you also add this feature to the python-experimental generator?
|
469be73
to
72baae9
Compare
@spacether well I know @gaborbernat is in favor of it, I was worried it would change the PR from a bugfix to a feature and slow it down (we're hitting issues with hanging in the kubernetes python client). I added it in, working on figuring out the test now. @spacether actually would you mind pointing me to where this sort of test should go? Struggling a bit to find the right place |
Tests added, wasn't totally sure if those are generated tests or not so let me know if I need to put them somewhere else |
@fabianvf you put them in the right place, thanks!
What do you think? |
@spacether probably makes sense to update the docs to encourage the use of the context manager, since there could be longer running programs that create/destroy multiple clients that could still be impacted |
e185e3d
to
32fb9fb
Compare
if self._pool: | ||
self._pool.close() | ||
self._pool.join() | ||
self._pool = None | ||
if hasattr(atexit, 'unregister'): |
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 atexit.unregister
doesn't exist in Python 2, so I added this guard
This removes the `__del__` function from the generated Python client, and replaces it with a `cleanup` function. When a ThreadPool is created, the cleanup function is registered with the `atexit` module. This fixes OpenAPITools#5093, where the API client could hang indefinitely at garbage collection.
32fb9fb
to
6880e3f
Compare
Can you update these doc templates to use with context managers for api_client?
|
@fabianvf can you run
Need to be updated |
@spacether thank you, fixed! |
@@ -8,22 +8,26 @@ from pprint import pprint | |||
{{#hasAuthMethods}} |
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.
Can you add a line above {{#hasAuthMethods}}
instantiating configuration?
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.
I believe configuration is instantiated in the {{> python_doc_auth_partial}}
snippet
@@ -7,51 +7,55 @@ from pprint import pprint | |||
{{#hasAuthMethods}} |
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.
Can you add a line above {{#hasAuthMethods}} instantiating configuration?
api_instance = {{{packageName}}}.{{{classname}}}({{{packageName}}}.ApiClient(configuration)) | ||
{{#allParams}}{{paramName}} = {{{example}}} # {{{dataType}}} | {{{description}}}{{^required}} (optional){{/required}}{{#defaultValue}} (default to {{{.}}}){{/defaultValue}} | ||
{{/allParams}} | ||
# Enter a context with an instance of the API client |
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.
Aren't these blocks {{#hasAuthMethods}}
/{{^hasAuthMethods}}
identical except for setting
configuration.host?
Please move the identical block contents underneath the {{/hasAuthMethods}}
on line 25
@@ -7,18 +7,20 @@ from pprint import pprint | |||
{{> python_doc_auth_partial}} | |||
# Defining host is optional and default to {{{basePath}}} |
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.
Do we need to instantiate configuration here?
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.
I believe configuration is instantiated in the {{> python_doc_auth_partial}}
snippet
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.
actually it looks like that snippet doesn't even exit in the experimental directory...
# Enter a context with an instance of the API client | ||
with {{{packageName}}}.ApiClient(configuration) as api_client: | ||
# Create an instance of the API class | ||
api_instance = {{{packageName}}}.{{{classname}}}(api_client) |
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.
Aren't these blocks {{#hasAuthMethods}}
/{{^hasAuthMethods}}
identical except for setting
configuration.host?
Please move the identical block contents underneath the {{/hasAuthMethods}}
on line 20
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.
Thank you for the PR. This looks great!
The last requests are non-blocking. This update can be merged in to master.
This is a followup to PR OpenAPITools#5094, which had a few unresolved comments at merge time. This reduces the amount of redundant lines in the api example doc templates, and ensures that referenced Configuration objects are actually instantiated.
* Reduce redundancy in python docs This is a followup to PR #5094, which had a few unresolved comments at merge time. This reduces the amount of redundant lines in the api example doc templates, and ensures that referenced Configuration objects are actually instantiated. * Regenerate samples
* Reduce redundancy in python docs This is a followup to PR OpenAPITools#5094, which had a few unresolved comments at merge time. This reduces the amount of redundant lines in the api example doc templates, and ensures that referenced Configuration objects are actually instantiated. * Regenerate samples
The implementation and tests were already picked up by the upstream OpenAPI generator [here](OpenAPITools/openapi-generator#5094). Patching in the tests here for correctness and clarity. Reference: kubernetes-client#1073 Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
The implementation and tests were already picked up by the upstream OpenAPI generator [here](OpenAPITools/openapi-generator#5094). Patching in the tests here for correctness and clarity. Reference: kubernetes-client#1073 Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
The implementation and tests were already picked up by the upstream OpenAPI generator [here](OpenAPITools/openapi-generator#5094). Patching in the tests here for correctness and clarity. Reference: kubernetes-client#1073 Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
The implementation and tests were already picked up by the upstream OpenAPI generator [here](OpenAPITools/openapi-generator#5094). Patching in the tests here for correctness and clarity. Reference: kubernetes-client#1073 Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
The implementation and tests were already picked up by the upstream OpenAPI generator [here](OpenAPITools/openapi-generator#5094). Patching in the tests here for correctness and clarity. Reference: kubernetes-client/python#1073 Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
This removes the
__del__
function from the generated Python client,and replaces it with a
cleanup
function. When a ThreadPool is created,the cleanup function is registered with the
atexit
module.This fixes #5093, where the API client could hang indefinitely at
garbage collection.
Related to swagger-api/swagger-codegen#10002
PR checklist
./bin/
(or Windows batch scripts under.\bin\windows
) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the code or mustache templates for a language ({LANG}
) (e.g. php, ruby, python, etc).master
,4.3.x
,5.0.x
. Default:master
.cc @taxpon @frol @mbohlool @cbornet @kenjones-cisco @tomplus @Jyhess @slash-arun @spacether