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 ?

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).

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.

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.

Proposed patch for as_completed(). #20369 fixes wait(), and behaviour is consistent between the two.

> 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.

> Could you please try to write a unit test.

Revised patch with unit test for as_completed().

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?)

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.

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.

Ah...ok, here is a patch that includes an update to Doc/library/concurrent.futures.rst as well.

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

New changeset 1dac8c954488 by Victor Stinner in branch 'default':
Issue #20367: Add Glenn Langford to Misc/ACKS
http://hg.python.org/cpython/rev/1dac8c954488

@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 :)

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).

@Victor: Thank you, and I appreciate all your advice! I am still learning the dev environment but hope to be helpful. :)

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

This one is still not merged in Tulip, right?

Correct, if you want to work on it, see http://code.google.com/p/tulip/issues/detail?id=114

messages: + msg209321

messages: + msg209102

messages: + msg209091