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

Threads leaked if executor is not shut down #87

Closed
rohanpm opened this issue Dec 19, 2018 · 0 comments
Closed

Threads leaked if executor is not shut down #87

rohanpm opened this issue Dec 19, 2018 · 0 comments

Comments

@rohanpm
Copy link
Owner

rohanpm commented Dec 19, 2018

The RetryExecutor and probably some others will leak threads if the executor is not shut down. Although it could be argued that the caller should shut down the executor, the concurrent.futures ThreadPoolExecutor manages to clean up threads without requiring a shut down, and it would be convenient if that worked for all of our executors too.

Demo script here: https://gist.github.com/rohanpm/117dd79f7fa1e806f721e54d66f67773

Produces output:

============== thread pool ======================
Created 10 futures
------------- threads ----------------
  MainThread daemon=False alive=True
  ThreadPoolExecutor-0_0 daemon=True alive=True
  ThreadPoolExecutor-0_2 daemon=True alive=True
  ThreadPoolExecutor-0_1 daemon=True alive=True
  ThreadPoolExecutor-0_3 daemon=True alive=True

About to GC 0
After GC 0
------------- threads ----------------
  MainThread daemon=False alive=True

About to GC 1
After GC 1
------------- threads ----------------
  MainThread daemon=False alive=True

About to GC 2
After GC 2
============== thread pool w/retry w/shutdown ==============
Created 10 futures
------------- threads ----------------
  MainThread daemon=False alive=True

About to GC 0
After GC 0
------------- threads ----------------
  MainThread daemon=False alive=True

About to GC 1
After GC 1
------------- threads ----------------
  MainThread daemon=False alive=True

About to GC 2
After GC 2
============== thread pool w/retry ==============
Created 10 futures
------------- threads ----------------
  MainThread daemon=False alive=True
  RetryExecutor daemon=True alive=True
  ThreadPoolExecutor-2_2 daemon=True alive=True
  ThreadPoolExecutor-2_1 daemon=True alive=True
  ThreadPoolExecutor-2_0 daemon=True alive=True

About to GC 0
After GC 0
------------- threads ----------------
  MainThread daemon=False alive=True
  RetryExecutor daemon=True alive=True
  ThreadPoolExecutor-2_2 daemon=True alive=True
  ThreadPoolExecutor-2_3 daemon=True alive=True
  ThreadPoolExecutor-2_1 daemon=True alive=True
  ThreadPoolExecutor-2_0 daemon=True alive=True

About to GC 1
After GC 1
------------- threads ----------------
  MainThread daemon=False alive=True
  RetryExecutor daemon=True alive=True
  ThreadPoolExecutor-2_2 daemon=True alive=True
  ThreadPoolExecutor-2_3 daemon=True alive=True
  ThreadPoolExecutor-2_1 daemon=True alive=True
  ThreadPoolExecutor-2_0 daemon=True alive=True

About to GC 2
After GC 2

With ThreadPoolExecutor alone, or with ThreadPool & retry & shutdown, threads created by the executors disappear. However, if retry is used and shutdown is not used, threads remain alive forever.

The ThreadPoolExecutor implementation shows careful usage of weakref allowing the executor to be collected while the threads are running. Presumably same approach should be used for all cases where our executors create threads.

rohanpm added a commit that referenced this issue Dec 26, 2018
Threads should hold only weak references to their executors and
should exit once the reference is lost. This allows threads to be
cleaned up even if shutdown() is never called on an executor.

Fixes #87
bors bot added a commit that referenced this issue Dec 26, 2018
88: Avoid thread leaks when shutdown() is not called r=rohanpm a=rohanpm

Threads should hold only weak references to their executors and
should exit once the reference is lost. This allows threads to be
cleaned up even if shutdown() is never called on an executor.

Fixes #87

Co-authored-by: Rohan McGovern <rohan@mcgovern.id.au>
rohanpm added a commit that referenced this issue Dec 26, 2018
Threads should hold only weak references to their executors and
should exit once the reference is lost. This allows threads to be
cleaned up even if shutdown() is never called on an executor.

Fixes #87
bors bot added a commit that referenced this issue Dec 26, 2018
88: Avoid thread leaks when shutdown() is not called r=rohanpm a=rohanpm

Threads should hold only weak references to their executors and
should exit once the reference is lost. This allows threads to be
cleaned up even if shutdown() is never called on an executor.

Fixes #87

Co-authored-by: Rohan McGovern <rohan@mcgovern.id.au>
@bors bors bot closed this as completed in #88 Dec 26, 2018
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

No branches or pull requests

1 participant