From 0b95b49401ad36c30ac5561ff94efc7db8013c2c Mon Sep 17 00:00:00 2001 From: "Amber H. Brown" Date: Tue, 2 Jul 2019 21:49:55 +1000 Subject: [PATCH 1/8] try this? --- synapse/util/logcontext.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/util/logcontext.py b/synapse/util/logcontext.py index 6b0d2deea040..6a7be324a6d2 100644 --- a/synapse/util/logcontext.py +++ b/synapse/util/logcontext.py @@ -22,6 +22,7 @@ See doc/log_contexts.rst for details on how this works. """ +import types import logging import threading @@ -544,6 +545,9 @@ def run_in_background(f, *args, **kwargs): # by synchronous exceptions, so let's turn them into Failures. return defer.fail() + if isinstance(res, types.CoroutineType): + res = defer.ensureDeferred(res) + if not isinstance(res, defer.Deferred): return res From 4e19ac57676960e9f7752707e856a5e6e6c4bdc1 Mon Sep 17 00:00:00 2001 From: "Amber H. Brown" Date: Tue, 2 Jul 2019 22:50:15 +1000 Subject: [PATCH 2/8] fix the bug as well --- synapse/http/server.py | 16 +++++++++++----- synapse/rest/media/v1/preview_url_resource.py | 1 + synapse/util/logcontext.py | 2 +- tests/rest/media/v1/test_url_preview.py | 12 ++++++++++++ 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index f067c163c131..4fe2661dda2b 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -353,17 +353,23 @@ def render(self, request): """ Render the request, using an asynchronous render handler if it exists. """ - render_callback_name = "_async_render_" + request.method.decode("ascii") + render_callback_name = "render_" + request.method.decode("ascii") + async_render_callback_name = "_async_" + render_callback_name - if hasattr(self, render_callback_name): + if hasattr(self, async_render_callback_name): + # Call the handler + callback = getattr(self, async_render_callback_name) + elif hasattr(self, render_callback_name): # Call the handler callback = getattr(self, render_callback_name) - defer.ensureDeferred(callback(request)) - - return NOT_DONE_YET else: super().render(request) + resp = callback(request) + if isinstance(resp, types.CoroutineType): + defer.ensureDeferred(resp) + return NOT_DONE_YET + def _options_handler(request): """Request handler for OPTIONS requests diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index 0337b64dc2b8..053346fb86d2 100644 --- a/synapse/rest/media/v1/preview_url_resource.py +++ b/synapse/rest/media/v1/preview_url_resource.py @@ -95,6 +95,7 @@ def __init__(self, hs, media_repo, media_storage): ) def render_OPTIONS(self, request): + request.setHeader(b"Allow", b"OPTIONS, GET") return respond_with_json(request, 200, {}, send_cors=True) @wrap_json_request_handler diff --git a/synapse/util/logcontext.py b/synapse/util/logcontext.py index 6a7be324a6d2..060c1134b55e 100644 --- a/synapse/util/logcontext.py +++ b/synapse/util/logcontext.py @@ -22,9 +22,9 @@ See doc/log_contexts.rst for details on how this works. """ -import types import logging import threading +import types from twisted.internet import defer, threads diff --git a/tests/rest/media/v1/test_url_preview.py b/tests/rest/media/v1/test_url_preview.py index 8fe596186640..976652aee820 100644 --- a/tests/rest/media/v1/test_url_preview.py +++ b/tests/rest/media/v1/test_url_preview.py @@ -460,3 +460,15 @@ def test_blacklisted_ipv6_range(self): "error": "DNS resolution failure during URL preview generation", }, ) + + def test_OPTIONS(self): + """ + OPTIONS returns the OPTIONS. + """ + request, channel = self.make_request( + "OPTIONS", "url_preview?url=http://example.com", shorthand=False + ) + request.render(self.preview_url) + self.pump() + self.assertEqual(channel.code, 200) + self.assertEqual(channel.json_body, {}) From dbbb4f151295d2a3089b4d2eb4a2e16beef14990 Mon Sep 17 00:00:00 2001 From: "Amber H. Brown" Date: Tue, 2 Jul 2019 22:51:26 +1000 Subject: [PATCH 3/8] changelog --- changelog.d/5593.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5593.bugfix diff --git a/changelog.d/5593.bugfix b/changelog.d/5593.bugfix new file mode 100644 index 000000000000..e981589ac3fa --- /dev/null +++ b/changelog.d/5593.bugfix @@ -0,0 +1 @@ +Fix regression in 1.1rc1 where OPTIONS requests to the media repo would fail. From 21e250e400789305a38afd41046c694243712e68 Mon Sep 17 00:00:00 2001 From: "Amber H. Brown" Date: Tue, 2 Jul 2019 23:43:27 +1000 Subject: [PATCH 4/8] fix --- synapse/http/server.py | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index 4fe2661dda2b..020c16d5f0ae 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -356,19 +356,30 @@ def render(self, request): render_callback_name = "render_" + request.method.decode("ascii") async_render_callback_name = "_async_" + render_callback_name - if hasattr(self, async_render_callback_name): - # Call the handler - callback = getattr(self, async_render_callback_name) - elif hasattr(self, render_callback_name): - # Call the handler - callback = getattr(self, render_callback_name) - else: - super().render(request) + # Try and get the async renderer + callback = getattr(self, async_render_callback_name, None) + + # If it doesn't exist, try and get a regular renderer + if not callback: + callback = getattr(self, render_callback_name, None) + + # No renderer for this request method. Fall back to Twisted's "not + # found" handling. + if not callback: + return super().render(request) resp = callback(request) + + # If it's a coroutine, turn it into a Deferred if isinstance(resp, types.CoroutineType): - defer.ensureDeferred(resp) - return NOT_DONE_YET + resp = defer.ensureDeferred(resp) + + # If it's a Deferred, return NOT_DONE_YET as it doesn't have a + # value yet + if not isinstance(resp, defer.Deferred): + return NOT_DONE_YET + + return resp def _options_handler(request): From b3aabae4fad1fac9f32501afa40aa176df955d49 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 3 Jul 2019 00:09:49 +1000 Subject: [PATCH 5/8] Update synapse/http/server.py Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/http/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index 020c16d5f0ae..df0c80143694 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -376,7 +376,7 @@ def render(self, request): # If it's a Deferred, return NOT_DONE_YET as it doesn't have a # value yet - if not isinstance(resp, defer.Deferred): + if isinstance(resp, defer.Deferred): return NOT_DONE_YET return resp From f3669bf88c5229f6dd4b5d39c7840ae0bb3c6164 Mon Sep 17 00:00:00 2001 From: "Amber H. Brown" Date: Wed, 3 Jul 2019 00:13:25 +1000 Subject: [PATCH 6/8] fix --- synapse/http/server.py | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index df0c80143694..860c116c384b 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -353,18 +353,12 @@ def render(self, request): """ Render the request, using an asynchronous render handler if it exists. """ - render_callback_name = "render_" + request.method.decode("ascii") - async_render_callback_name = "_async_" + render_callback_name + async_render_callback_name = "_async_render_" + request.method.decode("ascii") # Try and get the async renderer callback = getattr(self, async_render_callback_name, None) - # If it doesn't exist, try and get a regular renderer - if not callback: - callback = getattr(self, render_callback_name, None) - - # No renderer for this request method. Fall back to Twisted's "not - # found" handling. + # No async renderer for this request method. if not callback: return super().render(request) @@ -372,14 +366,9 @@ def render(self, request): # If it's a coroutine, turn it into a Deferred if isinstance(resp, types.CoroutineType): - resp = defer.ensureDeferred(resp) + defer.ensureDeferred(resp) - # If it's a Deferred, return NOT_DONE_YET as it doesn't have a - # value yet - if isinstance(resp, defer.Deferred): - return NOT_DONE_YET - - return resp + return NOT_DONE_YET def _options_handler(request): From 02ccd9285975a26fc1f91fdd3137f6be7c27ab76 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 2 Jul 2019 15:29:59 +0100 Subject: [PATCH 7/8] Add a couple of tests for run_in_background on coroutines also update the comment. --- synapse/util/logcontext.py | 5 +++-- tests/util/test_logcontext.py | 33 +++++++++++++++++++++------------ 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/synapse/util/logcontext.py b/synapse/util/logcontext.py index 060c1134b55e..9e1b53780405 100644 --- a/synapse/util/logcontext.py +++ b/synapse/util/logcontext.py @@ -529,8 +529,9 @@ def run_in_background(f, *args, **kwargs): return from the function, and that the sentinel context is set once the deferred returned by the function completes. - Useful for wrapping functions that return a deferred which you don't yield - on (for instance because you want to pass it to deferred.gatherResults()). + Useful for wrapping functions that return a deferred or coroutine, which you don't + yield or await on (for instance because you want to pass it to + deferred.gatherResults()). Note that if you completely discard the result, you should make sure that `f` doesn't raise any deferred exceptions, otherwise a scary-looking diff --git a/tests/util/test_logcontext.py b/tests/util/test_logcontext.py index 8adaee3c8d3d..8d69fbf11160 100644 --- a/tests/util/test_logcontext.py +++ b/tests/util/test_logcontext.py @@ -39,24 +39,17 @@ def _test_run_in_background(self, function): callback_completed = [False] - def test(): + with LoggingContext() as context_one: context_one.request = "one" - d = function() + + # fire off function, but don't wait on it. + d2 = logcontext.run_in_background(function) def cb(res): - self._check_test_key("one") callback_completed[0] = True return res - d.addCallback(cb) - - return d - - with LoggingContext() as context_one: - context_one.request = "one" - - # fire off function, but don't wait on it. - logcontext.run_in_background(test) + d2.addCallback(cb) self._check_test_key("one") @@ -105,6 +98,22 @@ def testfunc(): return self._test_run_in_background(testfunc) + def test_run_in_background_with_coroutine(self): + async def testfunc(): + self._check_test_key("one") + d = Clock(reactor).sleep(0) + self.assertIs(LoggingContext.current_context(), LoggingContext.sentinel) + await d + self._check_test_key("one") + + return self._test_run_in_background(testfunc) + + def test_run_in_background_with_nonblocking_coroutine(self): + async def testfunc(): + self._check_test_key("one") + + return self._test_run_in_background(testfunc) + @defer.inlineCallbacks def test_make_deferred_yieldable(self): # a function which retuns an incomplete deferred, but doesn't follow From cf122751e571abf4abda1f1523a0968d1dff6448 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 2 Jul 2019 18:50:36 +0100 Subject: [PATCH 8/8] update comments --- synapse/http/server.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index 860c116c384b..d993161a3e08 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -65,8 +65,8 @@ def wrap_json_request_handler(h): The handler method must have a signature of "handle_foo(self, request)", where "request" must be a SynapseRequest. - The handler must return a deferred. If the deferred succeeds we assume that - a response has been sent. If the deferred fails with a SynapseError we use + The handler must return a deferred or a coroutine. If the deferred succeeds + we assume that a response has been sent. If the deferred fails with a SynapseError we use it to send a JSON response with the appropriate HTTP reponse code. If the deferred fails with any other type of error we send a 500 reponse. """