bpo-27144: concurrent.futures as_complete and map iterators do not keep reference to returned object #1560

grzgrzgrz3 · 2017-05-12T18:42:53Z

https://bugs.python.org/issue27144

mention-bot · 2017-05-12T18:42:55Z

@grzgrzgrz3, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brianquinlan, @asvetlov and @ezio-melotti to be potential reviewers.

pitrou · 2017-07-03T07:42:00Z

Lib/concurrent/futures/_base.py

@@ -170,6 +170,17 @@ def _create_and_install_waiters(fs, return_when):

    return waiter

+
+def _yield_future(fs, waiter, ref_collect=()):


Can you give this function a more descriptive name?

pitrou · 2017-07-03T07:42:13Z

Lib/concurrent/futures/_base.py

+
+def _yield_future(fs, waiter, ref_collect=()):
+    while fs:
+        with fs[0]._condition:


Can you explain why you need to do this?

If your question is about line 176. Based on issue20319, changes on future waiters list should be locked.

I guess I don't understand why you need to remove the waiter. Previous code didn't do this.

If we do not remove waiter, second result set on future will trigger waiter and cause KeyError, future is already returned and reference cleared.

For example:

>>> from concurrent.futures import Future, as_completed >>> fs_finished = Future() >>> fs = Future() >>> fs_finished.set_result("") >>> for x in as_completed([fs_finished, fs, Future()]): ... fs.set_result(None) ... Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/grzegorz/cpython/Lib/concurrent/futures/_base.py", line 235, in as_completed pending.remove(future) KeyError: <Future at 0x7f566ff6fc20 state=finished returned NoneType>

This error occurs in both version.

However docstring for Future.set_result and Future.set_exception says:

Should only be used by Executor implementations and unit tests.

So maybe we should ignore this case?

If I'm understanding correctly then, yes, I would certainly consider it an error to call set_result twice.

Anyway, this part is not relate to origin issue, so i ll remove discussed code.

Maybe in future someone encounters it and create new issue, so it can be discussed there.

pitrou · 2017-07-03T07:42:47Z

Lib/concurrent/futures/_base.py

+            fs[0]._waiters.remove(waiter)
+
+        for future_list in ref_collect:
+            future_list.remove(fs[0])


If future_list is a list, then this is O(n)...

In this use case future_list will be always a set.
set().remove is O(1).
I will rename future_list to futures_collection

pitrou · 2017-07-03T07:43:37Z

Lib/concurrent/futures/_base.py

                    else:
-                        yield future.result(end_time - time.time())
+                        yield fs.pop(0).result(end_time - time.time())


If fs is a list, then this is O(n).

pitrou · 2017-07-03T07:43:55Z

Lib/concurrent/futures/_base.py

+
+        for future_list in ref_collect:
+            future_list.remove(fs[0])
+        yield fs.pop(0)


In this case fs is always a list, however I can modify this function to yield last element. I think order here does not matter. If order matter, reversing list is the option. What do you think?

Either reverse the list or make it a deque, as you prefer.

pitrou · 2017-07-03T07:44:40Z

Lib/concurrent/futures/process.py

@@ -357,6 +357,12 @@ def _check_system_limits():
    raise NotImplementedError(_system_limited)


+def _chain_from_iterable(iterable):


Why do you need this? Please add a comment and/or docstring.

pitrou · 2017-07-03T07:45:32Z

Lib/test/test_concurrent_futures.py

@@ -59,6 +59,10 @@ def my_method(self):
        pass


+def _map_fn(_):


Please give this function a more descriptive name, new_object perhaps?

pitrou · 2017-07-03T07:47:40Z

Thanks for caring about this issue. I think the proposed fix needs improvements, see comments.

grzgrzgrz3 · 2017-07-03T21:38:24Z

Thank you for review 👍. To be honest when i was writing it i do not think about complexity at all.

Please review new version. I have pushed with force, delete branch before pulling.

grzgrzgrz3 · 2017-07-03T21:41:47Z

Lib/concurrent/futures/_base.py

@@ -170,6 +170,17 @@ def _create_and_install_waiters(fs, return_when):

    return waiter

+
+def _yield_and_decref(fs, waiter, ref_collect=()):


This is my best, i could not came up with anything better. Maybe you can suggest name more descriptive.

Perhaps add a comment explaining why this function exists, then?

pitrou · 2017-07-04T09:17:54Z

Lib/test/test_concurrent_futures.py

+            # We don't particularly care what the default name is, just that
+            # it has a default name implying that it is a ThreadPoolExecutor
+            # followed by what looks like a thread number.
+            self.assertRegex(t.name, r'^.*ThreadPoolExecutor.*_[0-4]$')


Out of curiosity, why does your PR affect this?

My mistake, when rebasing.

pitrou · 2017-07-04T09:20:56Z

Lib/test/test_concurrent_futures.py

+        for result_object in self.executor.map(_dummy_object_fn, range(10)):
+            self.assertEqual(sys.getrefcount(result_object), 2)
+
+    def test_map_result_order(self):


Isn't this already tested by test_map? Or perhaps you just want to augment test_map with a chunksize-using test.

grzgrzgrz3 · 2017-07-26T17:37:13Z

Any update on review. Please have a look at last revision.

pitrou · 2017-08-29T21:25:35Z

Sorry for the delay @grzgrzgrz3. The PR now has conflicts, could you please resolve them?
You'll also need to add a NEWS entry using the blurb CLI tool.

reference to returned object

grzgrzgrz3 · 2017-08-30T09:40:08Z

Done.

pitrou · 2017-09-01T16:20:31Z

I'm trying to push some small changes to your branch. Crossing fingers.

…ying on sys.getrefcount() in tests.

pitrou · 2017-09-01T16:53:56Z

I'm merging this. Thank you very much!

…not keep reference to returned object (pythonGH-1560) * bpo-27144: concurrent.futures as_complie and map iterators do not keep reference to returned object * Some nits. Improve wordings in docstrings and comments, and avoid relying on sys.getrefcount() in tests. (cherry picked from commit 97e1b1c)

…not keep reference to returned object (GH-1560) (#3266) bpo-27144: concurrent.futures as_complie and map iterators do not keep reference to returned object (cherry picked from commit 97e1b1c)

* 'master' of https://github.com/python/cpython: (601 commits) remove check for bug last seem in Solaris 9 (python#3285) Change code owners for hashlib and ssl to the crypto team (python#3284) bpo-31281: Fix pathlib.Path incompatibility in fileinput (pythongh-3208) remove autoconf check for select() (python#3283) remove configure check for 'volatile' (python#3281) Add missing _sha3 module to Setup.dist (python#2395) bpo-12383: Also ignore __PYVENV_LAUNCHER__ (python#3278) bpo-9146: add the missing NEWS entry. (python#3275) Fix a c.f.as_completed() refleak previously introduced in bpo-27144 (python#3270) bpo-31185: Fixed miscellaneous errors in asyncio speedup module. (python#3076) remove a redundant lower in urllib.parse.urlsplit (python#3008) bpo-31323: Fix reference leak in test_ssl (python#3263) bpo-31250, test_asyncio: fix EventLoopTestsMixin.tearDown() (python#3264) bpo-31326: ProcessPoolExecutor waits for the call queue thread (python#3265) bpo-27144: concurrent.futures as_complete and map iterators do not keep reference to returned object (python#1560) bpo-31250, test_asyncio: fix dangling threads (python#3252) bpo-31217: Fix regrtest -R for small integer (python#3260) bpo-30096: Use ABC in abc reference examples (python#1220) bpo-30737: Update DevGuide links to new URL (pythonGH-3228) [Trivial] Remove now redundant assert (python#3245) ...

…ep reference to returned object (python#1560) * bpo-27144: concurrent.futures as_complie and map iterators do not keep reference to returned object * Some nits. Improve wordings in docstrings and comments, and avoid relying on sys.getrefcount() in tests.

ambv · 2017-09-29T20:37:57Z

Lib/concurrent/futures/_base.py

@@ -191,16 +205,18 @@ def as_completed(fs, timeout=None):
    if timeout is not None:
        end_time = timeout + time.time()

+    total_futures = len(fs)
+
    fs = set(fs)


This is a regression. The ordering of instructions here is wrong. Now fs must be a sequence to support the len(fs).

…ted()` This was possible before. pythonGH-1560 introduced a regression after 3.6.2 got released where only sequences were accepted now. This commit addresses this problem.

…completed()` (pythonGH-3830) This was possible before. pythonGH-1560 introduced a regression after 3.6.2 got released where only sequences were accepted now. This commit addresses this problem. (cherry picked from commit 574562c)

…ted()` (#3830) This was possible before. GH-1560 introduced a regression after 3.6.2 got released where only sequences were accepted now. This commit addresses this problem.

…completed()` (GH-3830) (#3831) This was possible before. GH-1560 introduced a regression after 3.6.2 got released where only sequences were accepted now. This commit addresses this problem. (cherry picked from commit 574562c)

…completed()` (pythonGH-3830) (python#3831) This was possible before. pythonGH-1560 introduced a regression after 3.6.2 got released where only sequences were accepted now. This commit addresses this problem. (cherry picked from commit 574562c)

Python issues: + python/cpython#1560 + python/cpython#3270 + python/cpython#3830

the-knights-who-say-ni added the CLA signed label May 12, 2017

pitrou changed the title ~~bpo-27144: concurrent.futures as_complie and map iterators do not keep~~ bpo-27144: concurrent.futures as_complete and map iterators do not keep Jul 3, 2017

pitrou changed the title ~~bpo-27144: concurrent.futures as_complete and map iterators do not keep~~ bpo-27144: concurrent.futures as_complete and map iterators do not keep reference to returned object Jul 3, 2017

pitrou reviewed Jul 3, 2017

View changes

grzgrzgrz3 force-pushed the fix-issue-27144 branch from 67e22b6 to f24c9d4 Compare Jul 3, 2017

grzgrzgrz3 commented Jul 3, 2017

View changes

pitrou reviewed Jul 4, 2017

View changes

grzgrzgrz3 force-pushed the fix-issue-27144 branch from f24c9d4 to 12dc30f Compare Jul 4, 2017

bpo-27144: concurrent.futures as_complie and map iterators do not keep

d581bba

reference to returned object

grzgrzgrz3 force-pushed the fix-issue-27144 branch from 12dc30f to d581bba Compare Aug 30, 2017

Some nits. Improve wordings in docstrings and comments, and avoid rel…

d67b22c

…ying on sys.getrefcount() in tests.

pitrou force-pushed the fix-issue-27144 branch from ae0262c to d67b22c Compare Sep 1, 2017

pitrou merged commit 97e1b1c into python:master Sep 1, 2017

ambv reviewed Sep 29, 2017

View changes

ambv mentioned this pull request Sep 29, 2017

bpo-31641: Allow arbitrary iterables in concurrent.futures.as_completed() #3830

Merged

miss-islington mentioned this pull request Sep 29, 2017

[3.6] bpo-31641: Allow arbitrary iterables in concurrent.futures.as_completed() (GH-3830) #3831

Merged

dalcinl added a commit to dalcinl/pythonfutures that referenced this pull request Oct 3, 2017

Backport fixes to as_completed and map iterators (bpo-27144)

61f5abb

Python issues: + python/cpython#1560 + python/cpython#3270 + python/cpython#3830

dalcinl mentioned this pull request Oct 3, 2017

Backport fixes to as_completed and map iterators (bpo-27144) agronholm/pythonfutures#66

Merged

agronholm pushed a commit to agronholm/pythonfutures that referenced this pull request Nov 29, 2017

Backport fixes to as_completed and map iterators (bpo-27144) (#66)

28fa404

Python issues: + python/cpython#1560 + python/cpython#3270 + python/cpython#3830

dependabot-preview bot mentioned this pull request May 17, 2018

Bump futures from 3.1.1 to 3.2.0 gita/BhagavadGita#4

Closed

dependabot-preview bot mentioned this pull request Aug 15, 2018

Bump futures from 3.0.3 to 3.2.0 DemocracyClub/yournextrepresentative#578

Closed

dependabot-preview bot mentioned this pull request Oct 11, 2018

Bump futures from 3.1.1 to 3.2.0 in /sdks/python yifanzou/beam#32

Closed

dependabot-preview bot mentioned this pull request Nov 6, 2018

Bump futures from 3.1.1 to 3.2.0 WikipediaLibrary/TWLight#164

Merged

This was referenced May 27, 2019

Bump futures from 3.1.1 to 3.2.0 JustinWingChungHui/okKindred#375

Closed

Bump futures from 3.0.5 to 3.2.0 MoveOnOrg/fb-to-redshift#25

Open

dependabot-preview bot mentioned this pull request Mar 23, 2020

Bump futures from 3.1.1 to 3.3.0 SickChill/sickchill#6142

Merged

bpo-27144: concurrent.futures as_complete and map iterators do not keep reference to returned object #1560

bpo-27144: concurrent.futures as_complete and map iterators do not keep reference to returned object #1560

grzgrzgrz3 commented May 12, 2017 •

edited by bedevere-bot

mention-bot commented May 12, 2017

pitrou Jul 3, 2017

pitrou Jul 3, 2017

grzgrzgrz3 Jul 3, 2017

pitrou Jul 4, 2017

grzgrzgrz3 Jul 4, 2017 •

edited

pitrou Jul 4, 2017

grzgrzgrz3 Jul 4, 2017

pitrou Jul 3, 2017

grzgrzgrz3 Jul 3, 2017

pitrou Jul 3, 2017

pitrou Jul 3, 2017

grzgrzgrz3 Jul 3, 2017

pitrou Jul 4, 2017

pitrou Jul 3, 2017

pitrou Jul 3, 2017

pitrou commented Jul 3, 2017

grzgrzgrz3 commented Jul 3, 2017

grzgrzgrz3 Jul 3, 2017

pitrou Jul 4, 2017

pitrou Jul 4, 2017

grzgrzgrz3 Jul 4, 2017

pitrou Jul 4, 2017

grzgrzgrz3 commented Jul 26, 2017

pitrou commented Aug 29, 2017

grzgrzgrz3 commented Aug 30, 2017

pitrou commented Sep 1, 2017

pitrou commented Sep 1, 2017

ambv Sep 29, 2017

		@@ -170,6 +170,17 @@ def _create_and_install_waiters(fs, return_when):

		return waiter


		def _yield_future(fs, waiter, ref_collect=()):

		@@ -357,6 +357,12 @@ def _check_system_limits():
		raise NotImplementedError(_system_limited)


		def _chain_from_iterable(iterable):

		@@ -170,6 +170,17 @@ def _create_and_install_waiters(fs, return_when):

		return waiter


		def _yield_and_decref(fs, waiter, ref_collect=()):

bpo-27144: concurrent.futures as_complete and map iterators do not keep reference to returned object #1560

bpo-27144: concurrent.futures as_complete and map iterators do not keep reference to returned object #1560

Conversation

grzgrzgrz3 commented May 12, 2017 • edited by bedevere-bot

mention-bot commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzgrzgrz3 Jul 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Jul 3, 2017

grzgrzgrz3 commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzgrzgrz3 commented Jul 26, 2017

pitrou commented Aug 29, 2017

grzgrzgrz3 commented Aug 30, 2017

pitrou commented Sep 1, 2017

pitrou commented Sep 1, 2017

Choose a reason for hiding this comment

grzgrzgrz3 commented May 12, 2017 •

edited by bedevere-bot

grzgrzgrz3 Jul 4, 2017 •

edited