element-hq / element-hq/synapse

`ObservableDeferred`'s API is a mess

Open
#11,390 0 comments 0 reactions 0 assignees View on GitHub
P3 T-Task
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.