New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
concurrent.futures.as_completed() fails when given duplicate Futures #64566
Comments
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? |
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, 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(). bpo-20369 fixes wait(), and behaviour is consistent between the two. |
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. |
Revised patch with unit test for as_completed(). |
Hum, you should also modify the documentation to explicit the You may add the same sentence in the asyncio.as_completed() + completed = [f for f in futures.as_completed( [f1,f1] ) ] You can just use list(futures.as_completed([f1,f1])). Please no space + self.assertEqual( len(completed), 1 ) No space around parenthesis (see the PEP-8): 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 PEP-8 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': |
New changeset 1dac8c954488 by Victor Stinner in branch 'default': |
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': |
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 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: