Consistently return a Deferred in the descriptor cache

[hawkowl]

No description provided.

[erikjohnston]

I have a horrible suspicion we might have been doing this on purpose so that we don't incur the cost of wrapping/unwrapping the deferred

[hawkowl]

@erikjohnston it returning bare values feels like it's only working on accident, tbh :S

[erikjohnston]

We so consistently use inlineCallbacks that it'd be fine. Though I agree its fairly awful

[hawkowl]

@erikjohnston it hid a bug in push_bulk_rules, where it didn't yield, only called, and relied on there being a cached value!

[hawkowl]

(which meant anything that called this made the tests fail as it was expecting a value, not a deferred)

[richvdh]

As Erik has said, this was somewhat deliberate: see #2075 which made it so. It's also worth noting that CacheListDescriptor's docstring makes this explicit, though apparently when I retrofitted that docstring I didn't also do the same for CacheDescriptor.

I do agree that an inconsistent return type is horrific and a source of fragility like this, though if Erik's claims on #2075 are to be believed, that may be outweighed by the performance hit.

On the other hand, I don't really understand why it makes so much difference. Surely the overhead of a completed Deferred should be negligible? It means that the next level up does a bit of extra work, but it's really not much.

In conclusion, I guess we should decide how much of a performance hit this would cause. If it's negligible, then I am very happy for us to start unwinding #2075 and go for consistent return types not only in CacheDescriptor, but also in CacheListDescriptor and ObservableDeferred.

[richvdh]

[alternatively, let's just fix the bug in push_bulk_rules and move on.]

[richvdh]

needs more thought as above

[hawkowl]

I'll have a bit more of a think about this when the async/await stuff comes to a head.