classification
Title: concurrent.futures ThreadPoolExecutor keeps unnecessary references to worker functions.
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, bquinlan, jnoller, lyapun, mark.dickinson, pitrou, python-dev, sbt, schmir
Priority: normal Keywords: patch

Created on 2012-10-19 10:15 by mark.dickinson, last changed 2012-11-03 13:52 by asvetlov. This issue is now closed.

Files
File name Uploaded Description Edit
kill_reference.diff mark.dickinson, 2012-10-19 10:15 review
kill_reference_2.diff mark.dickinson, 2012-10-20 12:30 review
kill_reference_3.diff asvetlov, 2012-11-01 13:13 review
issue16284_with_comments.diff lyapun, 2012-11-03 11:49 review
Messages (9)
msg173318 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-19 10:15
The ThreadPoolExecutor unnecessarily keeps references to _WorkItem objects.  With the attached patch (which lacks a test), all tests still pass, and the references are removed as soon as they're no longer needed.
msg173388 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-10-20 12:30
A new patch (with tests), and a fuller explanation:

At work, we've got Python talking to a customer's existing COM library; we're using Thomas Heller's 'comtypes' library to do that.  Unfortunately, comtypes depends quite a lot on __del__-time cleanup, so reference counting matters.  (I'm well aware that this isn't the recommended way to deal with resource cleanup in Python, but rewriting the existing infrastructure isn't a realistic option here.)

Anyway, it turned out that the concurrent.futures executors were indirectly holding onto references to COM objects, causing issues with our application.

The attached patch adds a few 'del' statements to remove references that are no longer needed.  For the ProcessExecutor, some of those 'del' statements had to go into the multiprocessing.Queue implementation.

The troublesome pattern (in both multiprocessing and futures) takes the form (simplified):


def my_worker_function(...):
    ...
    while <exit_condition_not_satisfied>:
        obj = blocking_wait_for_next_item()
        do_processing(obj)
    ...

The issue is that the reference to obj is kept until the completion of the next blocking wait call.  I'm suggesting just adding an extra 'del obj' after 'do_processing(obj)'.
msg173391 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-10-20 14:24
Sounds fine to me. You might want to make the test CPython-specific.
msg173435 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2012-10-21 09:06
The concurrent.futures stuff looks good to me.

Could you add a comment explaining why the delete is necessary? And, as Antoine said, the test should be CPython only.
msg173693 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-10-24 16:45
LGTM
msg174412 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-01 13:13
Updated patch to execute tests only for CPython.
msg174608 - (view) Author: Taras Lyapun (lyapun) * Date: 2012-11-03 11:49
Added comments to patch
msg174617 - (view) Author: Roundup Robot (python-dev) Date: 2012-11-03 13:36
New changeset 70cef0a160cf by Andrew Svetlov in branch 'default':
Issue #16284: Prevent keeping unnecessary references to worker functions in concurrent.futures ThreadPoolExecutor.
http://hg.python.org/cpython/rev/70cef0a160cf
msg174618 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-03 13:36
Committed. Thanks.
History
Date User Action Args
2012-11-03 13:52:51asvetlovsetstage: test needed -> resolved
2012-11-03 13:36:45asvetlovsetstatus: open -> closed
resolution: fixed
messages: + msg174618

stage: test needed
2012-11-03 13:36:11python-devsetnosy: + python-dev
messages: + msg174617
2012-11-03 11:49:23lyapunsetfiles: + issue16284_with_comments.diff
nosy: + lyapun
messages: + msg174608

2012-11-01 13:13:11asvetlovsetfiles: + kill_reference_3.diff
nosy: + asvetlov
messages: + msg174412

2012-10-24 16:45:58sbtsetmessages: + msg173693
2012-10-21 09:06:46bquinlansetmessages: + msg173435
2012-10-20 14:24:39pitrousetnosy: + pitrou
messages: + msg173391
2012-10-20 12:38:20mark.dickinsonsetnosy: + jnoller, sbt
2012-10-20 12:30:00mark.dickinsonsetfiles: + kill_reference_2.diff

messages: + msg173388
components: + Library (Lib)
2012-10-19 10:33:27schmirsetnosy: + schmir
2012-10-19 10:15:26mark.dickinsoncreate