Add validation to `BatchedSend` and convert to asyncio
- Dominant language
- Python
- Stars
- 1.7k
- Forks
- 778
- Avg merge
- 2h 50m
- Merged PRs (30d)
- 3
Description
`BatchedSend` has a few edge cases which can silently put it into a broken state. (See my review on https://github.com/dask/distributed/pull/5457 for an enumeration of its issues.) Despite being a core piece of networking infrastructure, it has little internal validation or documentation of its methods' contracts. This means that future contributors can easily make innocuous-looking refactors to code that uses `BatchedSend`, and introduce edge-case race conditions or deadlocks without receiving any error. This is exactly what happened in https://github.com/dask/distributed/pull/3493: a small change that would have failed simple assertions within `BatchedSend` introduced deadlocks (https://github.com/dask/distributed/issues/5480) which were not resolved for two years.
We attempted to do this in https://github.com/dask/distributed/pull/6329. However, that proved difficult to merge because `BatchedSend` is actually used in different ways, with different expectations, on the worker vs scheduler vs client (in large part, because it doesn't have this validation or clear contracts).
With https://github.com/dask/distributed/issues/6350, the primary edge-case use of `BatchedSend` has been eliminated (restarting an instance with a new comm object). It should now be pretty easy to add some internal state validation and documentation to `BatchedSend`, to prevent problems like this in the future. In the process, we can also modernize convert Tornado and `gen_coroutine` uses to asyncio.
Contributor guide
Assessment
This issue has not been assessed yet.