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

Cleanup ThreadPool with atexit rather than __del__ #1073

Merged
merged 2 commits into from
Feb 10, 2020

Conversation

fabianvf
Copy link
Contributor

@fabianvf fabianvf commented Feb 6, 2020

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 PR also allows the client to be used as a context manager, which
will automatically clean up after itself rather than having to wait til
process exit.

This fixes #1037, where the API client could hang indefinitely at
garbage collection.

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 6, 2020
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 6, 2020
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 PR also allows the client to be used as a context manager, which
will automatically clean up after itself rather than having to wait til
process exit.

This fixes issue kubernetes-client#1037, where the API client could hang indefinitely at
garbage collection.
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 6, 2020
@yliaog
Copy link
Contributor

yliaog commented Feb 6, 2020

could you add a test that can run successfully with this fix, and would fail without the fix?

@yliaog yliaog self-assigned this Feb 6, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 6, 2020
@yliaog
Copy link
Contributor

yliaog commented Feb 6, 2020

/test

@roycaihw
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianvf, roycaihw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0976d59 into kubernetes-client:master Feb 10, 2020
fabianvf pushed a commit to fabianvf/python that referenced this pull request Feb 11, 2020
Cleanup ThreadPool with atexit rather than __del__

(cherry picked from commit 0976d59)
fabianvf pushed a commit to fabianvf/python that referenced this pull request Feb 11, 2020
Cleanup ThreadPool with atexit rather than __del__

(cherry picked from commit 0976d59)
fabianvf pushed a commit to fabianvf/python that referenced this pull request Feb 11, 2020
Cleanup ThreadPool with atexit rather than __del__

(cherry picked from commit 0976d59)
k8s-ci-robot added a commit that referenced this pull request Feb 11, 2020
cherry-pick of #1073 to release-11.0: Fix python3 hang
k8s-ci-robot added a commit that referenced this pull request Feb 11, 2020
cherry-pick of #1073 to release-12.0: Fix python3 hang
k8s-ci-robot added a commit that referenced this pull request Feb 21, 2020
cherry-pick of #1073 to release-10.0: Fix python3 hang
@fabianvf fabianvf mentioned this pull request Feb 26, 2020
palnabarun added a commit to palnabarun/python that referenced this pull request Mar 12, 2020
palnabarun added a commit to palnabarun/python that referenced this pull request Mar 12, 2020
palnabarun added a commit to palnabarun/python that referenced this pull request Jun 22, 2020
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 <[email protected]>
palnabarun added a commit to palnabarun/python that referenced this pull request Jun 22, 2020
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 <[email protected]>
palnabarun added a commit to palnabarun/python that referenced this pull request Jul 16, 2020
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 <[email protected]>
palnabarun added a commit to palnabarun/python that referenced this pull request Aug 19, 2020
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 <[email protected]>
bh717 pushed a commit to bh717/python-dapp that referenced this pull request Apr 1, 2024
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GC hanging on API client cleanup
4 participants