element-hq / element-hq/synapse
`ObservableDeferred`'s API is a mess
- Dominant language
- Python
- Stars
- 4.6k
- Forks
- 600
- Avg merge
- 5d 22h
- Merged PRs (30d)
- 51
Description
This issue has been migrated from [#11390](https://github.com/matrix-org/synapse/issues/11390).
---
... and I hate it.
In particular:
* it doesn't `consumeErrors` by default, which in 99% of cases leads to log spam about unhandled failures
* it does some awful `__getattr__` / `__setattr__` hijinks to "transparently" pass through the methods on the underlying `Deferred`, which makes it hard to type correctly and generally leads to magical behaviour.
* I find it weird that you can fire an `ObservableDeferred` either by directly calling its `callback`/`errback` methods, or by calling them on the deferred you pass into it.
I would like to replace it with a much simpler implementation. Here is a proposal for how we can do it without rewriting everything at once.
* [x] Define an abstract base class which `ObservableDeferred` implements, called `AbstractObservableDeferred`, which just exposes the `observe` method:
```python
class AbstractObservableDeferred(Generic[T], metaclass=abc.ABCMeta):
@abc.abstractmethod
def observe(self) -> "Deferred[T]": ...
```
* [ ] Replace consumers (rather than creators) of `ObservableDeferred` with `AbstractObservableDeferred`
* [ ] Create a new class `SimpleObservableDeferred` which implementents `AbstractObservableDeferred`, and just has a `callback`, an `errback`, and an `observe` method. Both methods should return `None`.
* [ ] Replace uses of `ObservableDeferred` which currently create a brand new `Deferred` with `SimpleObservableDeferred`
* [ ] Create a new class `DeferredObserver` which wraps a `SimpleObservableDeferred`:
```python
class DeferredObserver(AbstractObservableDeferred[T]):
def __init__(self):
self._observable = SimpleObservableDeferred[T]()
def observe(self) -> Deferred[T]:
return self._observable.observe()
def chain_to_deferred(self, deferred: Deferred[T]) -> None:
deferred.addCallbacks(self._observable.callback, self._observable.errback)
```
* [ ] Replace uses of `ObservableDeferred` which chain to an existing Deferred with `DeferredObserver`
* [ ] Remove now-redundant `ObservableDeferred`
Contributor guide
Research direction
Start by locating ObservableDeferred and its consumers and creators, then compare the completed AbstractObservableDeferred proposal with the remaining checklist. The work is done when consumers use the abstract interface, SimpleObservableDeferred and DeferredObserver replace the existing creation and chaining patterns, and ObservableDeferred can be removed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100