Our present implementation of OperationFuture.add_done_callback has two branches with vastly different behavior:
If the operation is already done, it will immediately invoke the callback on the main process. This is in line with asyncio and concurrent.futures.
If the operation is not already done, it will fork and spawn a separate process to do a blocking poll on the operation. When the operation is complete, it will invoke the callback on the forked process.
The second behavior is problematic for several reasons:
It violates principle of least astonishment. A user does not expect their process to be forked and that the callback could potentially be called on a forked process. In fact, concurrent.futures.ProcessPoolExecutor goes out of its way to ensure that callbacks are executed on the process that starts the pool, not in the processes owned by the pool.
Forking behavior is undesirable. First and foremost, it causes an entire copy of the process just to handle a simple non-blocking poll. Additionally, forking a multithreaded process is problematic and gRPC's c-core uses threads by design. Finally, gRPC does not support being forked and barfs when the connection gets corrupted (by some saving grace, either our retries or gRPC's own logic, gRPC can re-establish the connection in the forked process).
We should just use a thread for this. There's no benefit to us using multiprocessing for this case.
Our present implementation of
OperationFuture.add_done_callback
has two branches with vastly different behavior:asyncio
andconcurrent.futures
.The second behavior is problematic for several reasons:
concurrent.futures.ProcessPoolExecutor
goes out of its way to ensure that callbacks are executed on the process that starts the pool, not in the processes owned by the pool.We should just use a thread for this. There's no benefit to us using multiprocessing for this case.
Bonus: we can drop
dill
as a dependency.