I'm on vacation. Will have a look at this as soon as I'm back.

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.45%. Comparing base (bc4f265) to head (2fa83a8).
⚠️ Report is 9 commits behind head on main.

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
- Coverage   93.63%   93.45%   -0.19%     
==========================================
  Files          14       14              
  Lines         550      565      +15     
  Branches       46       49       +3     
==========================================
+ Hits          515      528      +13     
- Misses         31       32       +1     
- Partials        4        5       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm on vacation. Will have a look at this as soon as I'm back.

No worries have a great rest of your vacation!

I've looked through your fix, and I think it looks like a good improvement. And all the tests pass without modification 😌

I've added five small refactoring commits that I think improves it even further. The first four I think are non-controversial, but I'd like to hear your thoughts about the last one (0c0a841).

@jodal Thanks yes I agree with the first four. The fifth one the only thing is that I wanted to avoid the _get_hook getting called while holding the lock, because it can cause the lock to be held for an arbitrarily long time (since it can be the user's own code running).

On the topic of the _get_hook mechanism: I think it should be enforced that, like set() and set_exception(), set_get_hook() can be called at most once. And it should be enforced that at most one the the three (set(), set_exception(), set_get_hook()) can be called. Also, I think the way it is right now, if the user's _get_hook ever returned None, (and I believe T is allowed to be the type of None) then _get_hook_result will remain None and the _get_hook would get called again on the next get(). So I think the _get_hook_result should be stored in something like Optional[tuple[T]] instead of just Optional[T].

I did the stupid thing of removing Python 3.9 support and upgrading the syntax to 3.10 with this PR open, so I had the pleasure of rebasing this PR now 😅

I think that it is the right choice to hold the lock while running _get_hook(). Imagine two callers, A and B, who calls future.get() at about the same time:

• For the first caller A, there's no difference between keeping the lock while calling the hook function or not. The time to get the result is the same.
• For the second caller B, if A releases the lock before calling the hook function, then B might proceed to the same point and call the hook function too. A and B will then have a race where the first one to finish gets to store its result for later callers.
• If instead the second caller B has to wait on the lock until A is done calling the hook function, the hook function is only called once and there is no doubt about which result ends up being stored. The experienced time for B waiting for A to finish and make the result available should be about the same (or less because of reduced resource contention) than doing the hook work twice.

I agree it could be a good thing to enforce that only calling one of the setters is allowed. I'll see about doing that in a follow-up PR.

Also, I think the way it is right now, if the user's _get_hook ever returned None, (and I believe T is allowed to be the type of None) then _get_hook_result will remain None and the _get_hook would get called again on the next get(). So I think the _get_hook_result should be stored in something like Optional[tuple[T]] instead of just Optional[T].

This should now be fixed in #239.

On the topic of the _get_hook mechanism: I think it should be enforced that, like set() and set_exception(), set_get_hook() can be called at most once. And it should be enforced that at most one the the three (set(), set_exception(), set_get_hook()) can be called.

This should now be fixed in #240.