ResponseCache: fix handling of completed results

Turns out that ObservableDeferred.observe doesn't return a deferred if the
result is already completed. Fix handling and improve documentation.
This commit is contained in:
Richard van der Hoff 2018-04-13 07:32:29 +01:00
parent b78395b7fe
commit 60f6014bb7

View File

@ -14,6 +14,8 @@
# limitations under the License. # limitations under the License.
import logging import logging
from twisted.internet import defer
from synapse.util.async import ObservableDeferred from synapse.util.async import ObservableDeferred
from synapse.util.caches import metrics as cache_metrics from synapse.util.caches import metrics as cache_metrics
from synapse.util.logcontext import make_deferred_yieldable, run_in_background from synapse.util.logcontext import make_deferred_yieldable, run_in_background
@ -48,15 +50,21 @@ class ResponseCache(object):
def get(self, key): def get(self, key):
"""Look up the given key. """Look up the given key.
Returns a deferred which doesn't follow the synapse logcontext rules, Can return either a new Deferred (which also doesn't follow the synapse
so you'll probably want to make_deferred_yieldable it. logcontext rules), or, if the request has completed, the actual
result. You will probably want to make_deferred_yieldable the result.
If there is no entry for the key, returns None. It is worth noting that
this means there is no way to distinguish a completed result of None
from an absent cache entry.
Args: Args:
key (hashable): key (hashable):
Returns: Returns:
twisted.internet.defer.Deferred|None: None if there is no entry twisted.internet.defer.Deferred|None|E: None if there is no entry
for this key; otherwise a deferred result. for this key; otherwise either a deferred result or the result
itself.
""" """
result = self.pending_result_cache.get(key) result = self.pending_result_cache.get(key)
if result is not None: if result is not None:
@ -73,19 +81,17 @@ class ResponseCache(object):
you should wrap normal synapse deferreds with you should wrap normal synapse deferreds with
logcontext.run_in_background). logcontext.run_in_background).
Returns a new Deferred which also doesn't follow the synapse logcontext Can return either a new Deferred (which also doesn't follow the synapse
rules, so you will want to make_deferred_yieldable it logcontext rules), or, if *deferred* was already complete, the actual
result. You will probably want to make_deferred_yieldable the result.
(TODO: before using this more widely, it might make sense to refactor
it and get() so that they do the necessary wrapping rather than having
to do it everywhere ResponseCache is used.)
Args: Args:
key (hashable): key (hashable):
deferred (twisted.internet.defer.Deferred): deferred (twisted.internet.defer.Deferred[T):
Returns: Returns:
twisted.internet.defer.Deferred twisted.internet.defer.Deferred[T]|T: a new deferred, or the actual
result.
""" """
result = ObservableDeferred(deferred, consumeErrors=True) result = ObservableDeferred(deferred, consumeErrors=True)
self.pending_result_cache[key] = result self.pending_result_cache[key] = result
@ -144,7 +150,7 @@ class ResponseCache(object):
self._name, key) self._name, key)
d = run_in_background(callback, *args, **kwargs) d = run_in_background(callback, *args, **kwargs)
result = self.set(key, d) result = self.set(key, d)
elif result.called: elif not isinstance(result, defer.Deferred) or result.called:
logger.info("[%s]: using completed cached result for [%s]", logger.info("[%s]: using completed cached result for [%s]",
self._name, key) self._name, key)
else: else: