classification
Title: concurrent.futures.as_completed() fails when given duplicate Futures
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.3, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: glangford, gvanrossum, haypo, mark.dickinson, python-dev, tim.peters, yselivanov
Priority: normal Keywords: patch

Created on 2014-01-23 15:00 by glangford, last changed 2014-02-06 00:07 by gvanrossum. This issue is now closed.

Files
File name Uploaded Description Edit
test_dupfuture.py glangford, 2014-01-23 15:00
issue20367.patch glangford, 2014-01-24 14:25 review
issue20367.patch glangford, 2014-01-24 15:34 review
issue20367.patch glangford, 2014-01-24 17:21 review
issue20367.patch glangford, 2014-01-26 14:20 review
Messages (19)
msg208952 - (view) Author: Glenn Langford (glangford) * Date: 2014-01-23 15:00
concurrent.futures.as_completed([f,f]) will yield f twice, then fail with a KeyError for a Future f which is not completed.

If the Future has already completed, as_completed([f,f]) will yield f once and does not trigger an exception.

What is the correct behaviour?
   as_completed( [f,f] ) -> yield f twice ?
   wait( [f,f], return_when=ALL_COMPLETED ) -> yield f twice ?
msg208956 - (view) Author: Glenn Langford (glangford) * Date: 2014-01-23 15:15
There is a subtlety in the as_completed() code which explains a lot - note that "finished" starts off as a set in the _AcquireFutures block. So if a Future f has already completed,
   as_completed( [f,f] ) 
will only yield f once, because f appears once in the finished set.

Later on when waiter events are processed, "finished" turns into a list because of the line:

finished = waiter.finished_futures

So any duplicates in that list will cause problems in pending.remove(Future).
msg208957 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-01-23 15:22
Since the new asyncio module has also a Future class and functions like as_completed(), this issue concerns also asyncio. concurrent.futures and asyncio should have the same behaviour.
msg208961 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-23 15:47
I think you the caller was wrong to pass in [f, f] in the first place.

In asyncio, the argument is converted into a set before using it, but there's still a bug if you pass it a list containing two references to the same coroutine -- it gets wrapped in two separate Futures. I think the better behavior is to convert to a set first and then map coroutines to Futures.

So concurrent.futures should also convert to a set first.
msg209081 - (view) Author: Glenn Langford (glangford) * Date: 2014-01-24 14:25
Proposed patch for as_completed(). #20369 fixes wait(), and behaviour is consistent between the two.
msg209084 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-01-24 14:28
> Proposed patch for as_completed().

Could you please try to write a unit test. The unit test should fail without the patch, and fail with the patch. Then create a new patch including the patch.

If it's tricky to write a reliable test reproducing the race condition, you can use unittest.mock to mock some objects.
msg209091 - (view) Author: Glenn Langford (glangford) * Date: 2014-01-24 15:34
> Could you please try to write a unit test.

Revised patch with unit test for as_completed().
msg209093 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-01-24 15:55
Hum, you should also modify the documentation to explicit the
behaviour. Example: "Duplicates futures are only yielded once".

You may add the same sentence in the asyncio.as_completed()
documentation. It looks like asyncio tests doesn't test as_completed()
with duplicated future. You may write a new patch to modify asyncio
doc and tests. It should be very similar.

+        completed = [f for f in futures.as_completed( [f1,f1] ) ]

You can just use list(futures.as_completed([f1,f1])). Please no space
around parenthesis (see the PEP 8).

+        self.assertEqual( len(completed), 1 )

No space around parenthesis (see the PEP 8):
self.assertEqual(len(completed), 1).

You may check the list value instead: self.assertEqual(completed, [f1])

(Why "f1" name? There is no f2, maybe rename to f?)
msg209102 - (view) Author: Glenn Langford (glangford) * Date: 2014-01-24 17:21
Thanks for the feedback. The new patch is modified for PEP8 with naming consistent with other concurrent tests. assertEqual I think is clearer by checking list length, so it is not changed. The docstring is updated.

I suggest asyncio be handled separately.
msg209267 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-26 01:55
LGTM. But you also need to update Doc/library/concurrent.futures.rst

I see this as a bugfix so it's not necessary to get this in before the beta 3 release tonight.

I will work on an asyncio patch and test.
msg209321 - (view) Author: Glenn Langford (glangford) * Date: 2014-01-26 14:20
Ah...ok, here is a patch that includes an update to Doc/library/concurrent.futures.rst as well.
msg209338 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-26 17:59
New changeset 58b0f3e1ddf8 by Guido van Rossum in branch 'default':
Fix issue #20367: concurrent.futures.as_completed() for duplicate arguments.
http://hg.python.org/cpython/rev/58b0f3e1ddf8
msg209359 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-26 22:34
New changeset 1dac8c954488 by Victor Stinner in branch 'default':
Issue #20367: Add Glenn Langford to Misc/ACKS
http://hg.python.org/cpython/rev/1dac8c954488
msg209360 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-01-26 22:36
@Guido: Why not fixing the issue in Python 3.3? You forgot to add Gleen Langford to Misc/ACKS!

@Gleen: Congrats for your first commit :)
msg209361 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-26 22:42
Sorry, I suppose it needs to be backported to 3.3.

If someone wants to do that, please do (I'm afraid I'd mess up the merge).
msg209367 - (view) Author: Glenn Langford (glangford) * Date: 2014-01-26 23:43
@Victor: Thank you, and I appreciate all your advice! I am still learning the dev environment but hope to be helpful. :)
msg209413 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-27 08:13
New changeset 791b69f9f96d by Victor Stinner in branch '3.3':
Issue #20367: Fix behavior of concurrent.futures.as_completed() for duplicate
http://hg.python.org/cpython/rev/791b69f9f96d
msg210352 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-05 23:26
This one is still not merged in Tulip, right?
msg210353 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-02-06 00:07
Correct, if you want to work on it, see http://code.google.com/p/tulip/issues/detail?id=114
History
Date User Action Args
2014-02-06 00:07:36gvanrossumsetmessages: + msg210353
2014-02-05 23:26:48yselivanovsetnosy: + yselivanov
messages: + msg210352
2014-01-27 08:14:07hayposetstatus: open -> closed
resolution: remind -> fixed
2014-01-27 08:13:52python-devsetmessages: + msg209413
2014-01-26 23:43:54glangfordsetmessages: + msg209367
2014-01-26 22:42:34gvanrossumsetstatus: closed -> open
resolution: fixed -> remind
messages: + msg209361
2014-01-26 22:36:24hayposetmessages: + msg209360
2014-01-26 22:34:03python-devsetmessages: + msg209359
2014-01-26 17:59:49gvanrossumsetstatus: open -> closed
resolution: fixed
stage: resolved
2014-01-26 17:59:19python-devsetnosy: + python-dev
messages: + msg209338
2014-01-26 14:20:26glangfordsetfiles: + issue20367.patch

messages: + msg209321
2014-01-26 01:55:28gvanrossumsetmessages: + msg209267
2014-01-24 17:21:44glangfordsetfiles: + issue20367.patch

messages: + msg209102
2014-01-24 15:55:05hayposetmessages: + msg209093
2014-01-24 15:34:04glangfordsetfiles: + issue20367.patch

messages: + msg209091
2014-01-24 14:28:54hayposetmessages: + msg209084
2014-01-24 14:25:13glangfordsetfiles: + issue20367.patch
keywords: + patch
messages: + msg209081
2014-01-23 15:47:38gvanrossumsetmessages: + msg208961
2014-01-23 15:22:40hayposetnosy: + gvanrossum
messages: + msg208957
2014-01-23 15:15:43glangfordsetmessages: + msg208956
2014-01-23 15:01:43glangfordsetnosy: + tim.peters, mark.dickinson, haypo
2014-01-23 15:00:48glangfordcreate