classification
Title: concurrent.futures `wait` and `as_completed` depend on private api
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Nathaniel Manista, asvetlov, bquinlan, bwhmather, gregory.p.smith, pitrou
Priority: normal Keywords: patch

Created on 2014-10-25 20:41 by bwhmather, last changed 2019-05-07 16:28 by bquinlan.

Files
File name Uploaded Description Edit
non-magic-waiters.patch bwhmather, 2014-10-25 20:41 `wait` and `as_completed` implemented using callbacks review
Messages (9)
msg230016 - (view) Author: Ben Mather (bwhmather) * Date: 2014-10-25 20:41
Adds `remove_done_callback` to `Future` object, removes `_waiters` attribute, and re-implements `wait` and `as_completed` using callbacks.

This makes it possible to extend or replace `Future` without having to mimic its private `_waiters` interface.

Currently waiters and callbacks are triggered at different points after a cancel (waiters in `set_condition_and_notify_cancel` and callbacks in `cancel`.)  This is a problem as it means that implementing `wait` and `as_completed` using `add_done_callback` will result in a behaviour change unless the behaviour of `add_done_callback` is changed instead. 

I don't believe the current behaviour is as documented anyway so I'm not sure if this is a problem.  See issue 22630.

I am a little uncertain about the O(n) implementation of `remove_done_callback` but can't imagine a situation where it would be a problem.
msg233896 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2015-01-12 22:58
-1.
Sorry, I don't see the reason for making custom `Future` class.

Can you elaborate?
msg251218 - (view) Author: Ben Mather (bwhmather) * Date: 2015-09-21 11:04
Sorry for slow response.  I completely missed your reply.

I was working on a project that used a concurrent.futures thread pool but also needed to listen to responses from a chip-and-pin card reader.

Payment sessions operated in three phases:
  - check with card reader that payment is possible
  - save payment to database as if it has happened (always best to undercharge in the case of an error)
  - ask the payment provider to commit the payment

Up until confirmation is received, it is entirely possible to cancel the session.
Once confirmation is received the session starts trying to commit and you can only really wait for it to finish and then roll back (or break everything).

This maps pretty well to the interface of future (though with very different plumbing and with work happening before cancellation stops working) and it made sense to try to write it so that it could interoperate with futures representing tasks running on the thread pool.

I tried to add a method which returned a plain concurrent.futures.Future object but couldn't find a way to hook into the cancel method without introducing loads of race conditions.

Really it just seemed wrong that I had an object that quacked like a duck but wasn't a duck because real ducks communicated over a hidden side channel.

The card reader library is open source and mostly functional.  The class is in:
    https://github.com/bwhmather/python-payment-terminal/blob/develop/payment_terminal/drivers/bbs/payment_session.py

I'm sure there are other places where this sort of interoperability would be useful.
msg294568 - (view) Author: Nathaniel Manista (Nathaniel Manista) Date: 2017-05-26 20:21
gRPC Python is strongly affected by this as well. We use grpc.Future objects (https://github.com/grpc/grpc/blob/e5f99b587338164230b0b2121d242fb015c2ae5a/src/python/grpcio/grpc/__init__.py#L50) to represent RPCs taking place asynchronously. Despite our grpc.Futures being interface-compatible with concurrent.futures.Future objects, we (in our tests) and our users (in their applications) cannot make use of the concurrent.futures.as_completed function (https://github.com/grpc/grpc/blob/e5f99b587338164230b0b2121d242fb015c2ae5a/src/python/grpcio_tests/tests/unit/_rpc_test.py#L388 shows the "wrapping" that has to be done to work around the problem).

It's not at all clear why as_completed shouldn't work with any future objects that are public-interface-compatible with concurrent.futures.Future objects.

I would be happy to reimplement the function if my change would be accepted. I part ways with this issue's original reporter in that I don't see the need to widen the public API any further with a "remove_done_callback" method... but maybe I'm missing some small detail?
msg303567 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-10-02 22:58
any thoughts on this Antoine?
msg303580 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-10-03 06:59
This looks like a good idea.  Unfortunately, the patch is most certainly outdated.
msg303585 - (view) Author: Ben Mather (bwhmather) * Date: 2017-10-03 08:53
The patch is indeed a little outdated, but I would be happy to pick it and get it working again.

First we need a resolution to #22630 though.
Currently calling `cancel` will invoke callbacks, but waiters won't be triggered until `set_running_or_notify_cancel` is called.

If both are implemented using the same mechanism, then both will need to be cancelled at the same time.  This means that the behaviour of one the other will need to be changed in a backwards incompatible way.

Does anyone have a preference for where we should send notification of cancellation?
Is the breakage significant enough to kill this patch?
msg303589 - (view) Author: Ben Mather (bwhmather) * Date: 2017-10-03 09:41
@Nathaniel
RE `remove_done_callback`:  This was added as an optimisation to allow the `as_completed` iterator to remove callbacks for future that it has visited instead of having to store a separate set of visited futures and ignore them when their callbacks are triggered.

This matches how the private waiters mechanism works.

It might be worth submitting the core of this change without `remove_done_callback` and then discussing it later as a separate optimisation.  I will experiment.
msg341776 - (view) Author: Brian Quinlan (bquinlan) * (Python committer) Date: 2019-05-07 16:28
How did the experiment go? Are people still interested in this?
History
Date User Action Args
2019-05-07 16:28:33bquinlansetmessages: + msg341776
2017-10-03 09:41:03bwhmathersetmessages: + msg303589
2017-10-03 09:33:55pitrousetversions: + Python 3.7, - Python 3.4, Python 3.5, Python 3.6
2017-10-03 08:53:59bwhmathersetmessages: + msg303585
2017-10-03 06:59:15pitrousetmessages: + msg303580
2017-10-02 22:58:24gregory.p.smithsetnosy: + pitrou, gregory.p.smith

messages: + msg303567
title: `wait` and `as_completed` depend on private api -> concurrent.futures `wait` and `as_completed` depend on private api
2017-05-26 20:21:27Nathaniel Manistasetnosy: + Nathaniel Manista
messages: + msg294568
2015-09-21 11:04:53bwhmathersetmessages: + msg251218
2015-01-12 22:58:48asvetlovsetnosy: + asvetlov
messages: + msg233896
2014-10-26 02:25:20ned.deilysetnosy: + bquinlan
2014-10-25 20:41:39bwhmathercreate