gh-131466: concurrent.futures.Executor.map: avoid temporarily exceeding buffersize while collecting the next result

[ebonnal]

Context recap:

If we have:

results: Iterator = executor.map(fn, iterable, buffersize=buffersize)

What happens when calling next(results):

1. fetch the next arg from interable and put a task for fn(arg) in the buffer
2. wait for the next result to be available
3. yield the collected result

-> During step 2. there is buffersize + 1 buffered tasks.

This PR swaps steps 1. and 2. so that buffersize is never exceeded, even during next.

• Issue: concurrent.futures.Executor.map temporarily exceeds its buffersize while collecting the next result #131466

[ebonnal]

to be discussed: this could be replaced by a lighter yield fs.pop()._result because the prior call to _result_or_cancel guarantees that at this point the result is available.

[picnixz]

While I understand that we could possibly exceed buffersize while collecting the next result, is there a real-word use case where it would really cause an issue? the reason is that we access to fs[-1] and then do fs.pop().

I see that have a del fut in _result_or_cancel() but can you confirm that it's sufficient to not hold any reference to the yet-to-be-popped future?

[picnixz]

Asking Gregory as well since he's the mp expert c:

[ebonnal]

@picnixz sorry I re-asked your review because you made me realize that we actually don't need _result_or_cancel anymore:

test_executor_map_current_future_cancel introduced in #95169 does not break anymore because now if the fs[-1].result() access fails, the future is still in fs (not popped out like before) and it will be properly cancelled as part of the result_iterator's finally block.

I'm digging deeper into #95169 's context to check if I miss any non-tested scenario, especially regarding this:

    finally:
        # Break a reference cycle with the exception in self._exception
        del fut

[picnixz]

especially regarding this:

yes, that's what I wanted to ask, but I'm not an expert here so i'll let you investigate first c:

[ebonnal]

Hi @graingert!
Context:
As a side effect, this PR may remove the need for _result_or_cancel (introduced in #95169). If fetching the next result raises a TimeoutError, its future will still be in fs and will be properly cancelled by the result_iterator's finally block.

Question:
Do you remember in which scenario the del fut was required? Removing it in the current main does not break any tests 🤔

[graingert]

This is if fut.result() raises an exception there's a reference cycle where fut.exception().__traceback__ -> fut.exception()

Probably worth adding a test, a git grep for no_other_refs will find a similar one

[ebonnal]

Thank you! Will add the test 🫡

[ebonnal]

fyi #131701 adds the test @graingert @picnixz 🙏🏻