Interpreter exit blocks waiting for futures of shut-down ThreadPoolExecutors

[hniksic]

BPO	36780
Nosy	@brianquinlan, @pitrou, @hniksic, @asvetlov, @Bluehorn, @aeros
PRs	gh-80961: Add daemon parameter to ThreadPoolExecutor #13250
Files	pool-shutdown

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2019-05-03.09:28:03.892>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'Interpreter exit blocks waiting for futures of shut-down ThreadPoolExecutors'
updated_at = <Date 2020-04-15.02:39:49.873>
user = 'https://github.com/hniksic'

bugs.python.org fields:

activity = <Date 2020-04-15.02:39:49.873>
actor = 'aeros'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2019-05-03.09:28:03.892>
creator = 'hniksic'
dependencies = []
files = ['48297']
hgrepos = []
issue_num = 36780
keywords = ['patch']
message_count = 19.0
messages = ['341329', '341515', '341573', '341630', '341734', '341737', '341992', '342259', '342260', '342261', '366372', '366415', '366419', '366421', '366424', '366431', '366437', '366439', '366479']
nosy_count = 6.0
nosy_names = ['bquinlan', 'pitrou', 'hniksic', 'asvetlov', 'torsten', 'aeros']
pr_nums = ['13250']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36780'
versions = ['Python 3.7', 'Python 3.8']

[hniksic]

At interpreter shutdown, Python waits for all pending futures of all executors to finish. There seems to be no way to disable the wait for pools that have been explicitly shut down with pool.shutdown(wait=False). The attached script demonstrates the issue.

In our code the futures are running blocking network calls that can be canceled by the user. The cancel action automatically cancels the pending futures and informs the running ones that they should exit. The remaining futures are those whose callables are "stuck" in network calls with long or infinite timeouts, such as reading from a non-responding network filesystem. Since those can't be interrupted, we use pool.shutdown(wait=False) to disown the whole pool. This works nicely, until the application exit, at which point it blocks trying to wait for the pending futures to finish. This can take an arbitrary amount of time, possibly never finishing.

A similar question has come up on StackOverflow, with the only answer recommending to unregister concurrent.futures.thread._python_exit: https://stackoverflow.com/q/48350257/1600898 . We are ourselves using a similar hack, but we would like to contribute a proper way to disown an executor.

The current behavior is explicitly documented, so presumably it can't be (easily) changed, but we could add a "disown" keyword argument to shutdown(), or a new disown() method, which would serve to explicitly disable the waiting. If this is considered desirable, I will create a pull request.

[brianquinlan]

> The current behavior is explicitly documented, so presumably
> it can't be (easily) changed

And it isn't clear that it would be desirable to change this even if it were possible - doing structured resource clean-up seems consistent with the rest of Python.

That being said, I can see the user case for this.

If you added this as an argument to shutdown() then you'd probably also have to add it as an option to the constructors (for people using Executors as context managers). But, if you have to add it to the constructor anyway, you may as well *only* add it to the constructor.

I'll let an active maintainer weigh in on whether, on balance, they think that this functionality is worth complicating the API.

[brianquinlan]

OK, I completely disagree with my statement:

"""If you added this as an argument to shutdown() then you'd probably also have to add it as an option to the constructors (for people using Executors as context managers). But, if you have to add it to the constructor anyway, you may as well *only* add it to the constructor."""

This functionality wouldn't apply to context manager use (since the point of the context manager is to ensure resource clean-up). So maybe another argument to shutdown() would make sense

[hniksic]

I agree with the last comment. The disowning functionality is only used in specific circumstances, so it's perfectly fine to keep the functionality as a shutdown flag. I also agree that the change cannot be *unconditional*, for backward compatibility if not for other reasons.

The StackOverflow question, and more importantly the existence of shutdown(wait=False), suggest that there are legitimate cases when one doesn't want to wait for all running futures to finish. If a flag to shutdown() is considered to complicate the API, then perhaps we could add an opt-out by subclassing the executor and overriding a semi-private method.

Currently there seems to be no way to just abandon the thread pool. Since user threads don't and never will support cancellation, the options are:

1. add the disown option to shutdown, as suggested
2. monkey-patch concurrent.futures to not block at shutdown
3. make functions executed by the pool externally cancellable
4. roll our own thread pool

#1 is suggested by this issue, and #2 is what we do now, but it's obviously unacceptable in the long term.

#3 is infeasible because the functions we submit to the pool heavily rely on http (through "requests") and database communication, which don't support user-driven cancellation.

#4 would be technically possible, but it would require reimplementing half of concurrent.futures (badly), just for the purpose of being able to get rid of the mandatory wait at interpreter exit.

[brianquinlan]

Hey Hrvoje,

I agree that #1 is the correct approach. disown might not be the best name - maybe allow_shutdown or something. But we can bike shed about that later.

Are you interested in writing a patch?

[hniksic]

Are you interested in writing a patch?

Yes - I wanted to check if there is interest in the feature before I commit time to write the patch, documentation, tests, etc. (And also I wanted to check if there's a better way to do it.)

In any case, thanks for picking up on this.

disown might not be the best name - maybe allow_shutdown or something.

Disown is an admittedly obscure reference to the shell built-in of the same name (https://tinyurl.com/qfn8ao7). The idea is to get the point across that the pool is truly abandoned, and its running futures are left to their own devices. Maybe wait_at_exit would be a clearer name:

    pool.shutdown(wait=False, wait_at_exit=True)

[brianquinlan]

We can bike shed over the name in the PR ;-)

[asvetlov]

Should wait_on_exit be True only on application exit?
Do application exit means interpreter shutdown (sys.is_finalizing() == True)?

[hniksic]

@asvetlov The idea of the new flag is to disable any subsequent waiting for futures after ThreadPoolExecutor.shutdown(wait=False) returns.

Currently the additional waiting is implemented using "atexit", so I assumed it referred to process exit. (The documentation at https://docs.python.org/3/library/atexit.html doesn't seem to specify precisely when the callbacks are executed.) But looking at the implementation of atexit, it seems that the atexit callbacks are actually called from Py_FinalizeEx.

I think wait_at_exit is descriptive because in most cases the process exit and interpreter shutdown will correlate, but I can still update the docs to make it clearer what "exit" refers to. We should just avoid the word "shutdown" in the flag name to avoid confusion with executor shutdown.