Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix make_deferred_yieldable to work with coroutines #6505

Merged
merged 3 commits into from
Dec 10, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/6505.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make `make_deferred_yieldable` to work with async/await.
9 changes: 8 additions & 1 deletion synapse/logging/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
See doc/log_contexts.rst for details on how this works.
"""

import inspect
import logging
import threading
import types
Expand Down Expand Up @@ -612,7 +613,8 @@ def run_in_background(f, *args, **kwargs):


def make_deferred_yieldable(deferred):
"""Given a deferred, make it follow the Synapse logcontext rules:
"""Given a deferred (or coroutine), make it follow the Synapse logcontext
rules:

If the deferred has completed (or is not actually a Deferred), essentially
does nothing (just returns another completed deferred with the
Expand All @@ -624,6 +626,11 @@ def make_deferred_yieldable(deferred):

(This is more-or-less the opposite operation to run_in_background.)
"""
if inspect.isawaitable(deferred):
# If we're given a coroutine we need to convert it to a deferred so that
# we can attach callbacks (and not immediately return).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that's the reason we need to convert it to a deferred (we can do similar things by awaiting awaitables).

What it does do is to start running the coroutine and find out if it has finished or not, which saves us clearing and immediately restoring the logcontext in the common case that the coroutine completes synchronously.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, fair. I guess I was thinking of this in terms of converting to a future/deferred so that we could put the callbacks on it (since you can't do that on raw coroutines). Will reword

deferred = defer.ensureDeferred(deferred)

if not isinstance(deferred, defer.Deferred):
return deferred

Expand Down
24 changes: 24 additions & 0 deletions tests/util/test_logcontext.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,30 @@ def test_nested_logging_context(self):
nested_context = nested_logging_context(suffix="bar")
self.assertEqual(nested_context.request, "foo-bar")

@defer.inlineCallbacks
def test_make_deferred_yieldable_with_await(self):
# an async function which retuns an incomplete coroutine, but doesn't
# follow the synapse rules.

async def blocking_function():
d = defer.Deferred()
reactor.callLater(0, d.callback, None)
await d

sentinel_context = LoggingContext.current_context()

with LoggingContext() as context_one:
context_one.request = "one"

d1 = make_deferred_yieldable(blocking_function())
# make sure that the context was reset by make_deferred_yieldable
self.assertIs(LoggingContext.current_context(), sentinel_context)

yield d1

# now it should be restored
self._check_test_key("one")


# a function which returns a deferred which has been "called", but
# which had a function which returned another incomplete deferred on
Expand Down