gh-108518: Make concurrent.futures.Executor.map() consistent with built-in map() - #109497
Conversation
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Good code, but slightly over-engineered. I think that it will be better after simplification.
And there is a regression (removing the close() method).
| __class_getitem__ = classmethod(types.GenericAlias) | ||
|
|
||
|
|
||
| class _FutureResult(object): |
There was a problem hiding this comment.
It looks over-engineered. Why not simply use a result-exception tuple?
| @classmethod | ||
| def from_generator(cls, gen): | ||
| return cls(gen) |
There was a problem hiding this comment.
It is redundant. You can simply use constructor.
| return False | ||
|
|
||
|
|
||
| class _MapResultIterator(object): |
There was a problem hiding this comment.
(object) is redundant.
Current iterator has the close() method, calling which cancels all futures. It is a useful feature. Please add a close() method in a new class. I do not think that it is worth to implement send() and throw(), they are not so useful and are rather an implementation detail.
| ) as gen: | ||
| with self.assertRaises(TimeoutError): | ||
| next(gen) | ||
| iterator = pool.map(log_n_wait, ["second"], timeout=0) |
There was a problem hiding this comment.
This code should continue to work without changes.
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I resolved conflicts, fixed some errors, simplified code, added the close method, added docs and more tests. TimeoutError immediately cancels all calls.
Now this PR LGTM.
@pitrou, @brianquinlan, @vstinner, @gpshead, could anybody of your please take a look at this if you have a time?
|
This PR is stale because it has been open for 30 days with no activity. |
# Conflicts: # Lib/concurrent/futures/_base.py # Lib/concurrent/futures/process.py # Lib/test/test_concurrent_futures/executor.py
Documentation build overview
|
2 participants
The current behavior of
concurrent.futures.Executor.map()is not consistent with documentation:When a
nextcall raises an Exception, you can not get any subsequent Future result, even if the computation has completed and returns a valid result.If it is expected or don't consider to make changes to this behavior, I'd like to make a PR to update the relevant docs.