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

Can you give this function a more descriptive name?

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

Lib/concurrent/futures/_base.py

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:46Z

Lib/concurrent/futures/_base.py

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

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

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

Lib/concurrent/futures/_base.py

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

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

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

Lib/test/test_concurrent_futures.py

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

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

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

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


+    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 reviewed changes

Lib/concurrent/futures/_base.py Outdated

Copy link

Member

pitrou Jul 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give this function a more descriptive name?

pitrou reviewed Jul 3, 2017

View reviewed changes

Lib/concurrent/futures/_base.py Outdated

Copy link

Member

pitrou Jul 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pitrou reviewed Jul 3, 2017

View reviewed changes

Lib/concurrent/futures/process.py Outdated

Copy link

Member

pitrou Jul 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pitrou reviewed Jul 3, 2017

View reviewed changes

Lib/test/test_concurrent_futures.py Outdated

Copy link

Member

pitrou Jul 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

grzgrzgrz3 force-pushed the fix-issue-27144 branch from 67e22b6 to f24c9d4 Compare July 3, 2017 21:32

grzgrzgrz3 commented Jul 3, 2017

View reviewed changes

pitrou reviewed Jul 4, 2017

View reviewed changes

grzgrzgrz3 force-pushed the fix-issue-27144 branch from f24c9d4 to 12dc30f Compare July 4, 2017 14:33

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 August 30, 2017 09:38

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 September 1, 2017 16:33

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

ambv reviewed Sep 29, 2017

View reviewed 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_deprecated#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

Uh oh!

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

Uh oh!

Conversation

grzgrzgrz3 commented May 12, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grzgrzgrz3 Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jul 3, 2017

Uh oh!

grzgrzgrz3 commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grzgrzgrz3 commented Jul 26, 2017

Uh oh!

pitrou commented Aug 29, 2017

Uh oh!

grzgrzgrz3 commented Aug 30, 2017

Uh oh!

pitrou commented Sep 1, 2017

Uh oh!

pitrou commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

grzgrzgrz3 commented May 12, 2017 •

edited by bedevere-bot

Loading

grzgrzgrz3 Jul 4, 2017 •

edited

Loading