From f880e80080d5d63b3161539bf5ab28db5c87c7a2 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 16 Aug 2023 12:47:03 +0100 Subject: [PATCH 1/7] Make synapse.logging.context lint w/ twisted trunk I think the chang to `_set_context_cb` was the important thing, but I think the changes to `run_in_background` help make this clearer. --- synapse/logging/context.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/synapse/logging/context.py b/synapse/logging/context.py index f62bea968fe4..64c6ae451208 100644 --- a/synapse/logging/context.py +++ b/synapse/logging/context.py @@ -809,23 +809,24 @@ def run_in_background( # type: ignore[misc] # `res` may be a coroutine, `Deferred`, some other kind of awaitable, or a plain # value. Convert it to a `Deferred`. + d: "defer.Deferred[R]" if isinstance(res, typing.Coroutine): # Wrap the coroutine in a `Deferred`. - res = defer.ensureDeferred(res) + d = defer.ensureDeferred(res) elif isinstance(res, defer.Deferred): - pass + d = res elif isinstance(res, Awaitable): # `res` is probably some kind of completed awaitable, such as a `DoneAwaitable` # or `Future` from `make_awaitable`. - res = defer.ensureDeferred(_unwrap_awaitable(res)) + d = defer.ensureDeferred(_unwrap_awaitable(res)) else: # `res` is a plain value. Wrap it in a `Deferred`. - res = defer.succeed(res) + d = defer.succeed(res) - if res.called and not res.paused: + if d.called and not d.paused: # The function should have maintained the logcontext, so we can # optimise out the messing about - return res + return d # The function may have reset the context before returning, so # we need to restore it now. @@ -843,8 +844,8 @@ def run_in_background( # type: ignore[misc] # which is supposed to have a single entry and exit point. But # by spawning off another deferred, we are effectively # adding a new exit point.) - res.addBoth(_set_context_cb, ctx) - return res + d.addBoth(_set_context_cb, ctx) + return d T = TypeVar("T") @@ -877,7 +878,7 @@ def make_deferred_yieldable(deferred: "defer.Deferred[T]") -> "defer.Deferred[T] ResultT = TypeVar("ResultT") -def _set_context_cb(result: ResultT, context: LoggingContext) -> ResultT: +def _set_context_cb(result: ResultT, context: LoggingContextOrSentinel) -> ResultT: """A callback function which just sets the logging context""" set_current_context(context) return result From de25f6a0df38292ca50812396b02015fd3db8b67 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 16 Aug 2023 13:29:46 +0100 Subject: [PATCH 2/7] Make synapse.handlers.message work w/ twisted trunk --- synapse/handlers/message.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index d485f21e49f1..8a65bb648a10 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1474,21 +1474,23 @@ async def handle_new_client_event( # We now persist the event (and update the cache in parallel, since we # don't want to block on it). - event, context = events_and_context[0] + # Note: mypy gets confused if we inline dl and check with twisted#11770. + # Some kind of bug in mypy's deduction? + dl = ( + run_in_background( + self._persist_events, + requester=requester, + events_and_context=events_and_context, + ratelimit=ratelimit, + extra_users=extra_users, + ), + run_in_background( + self.cache_joined_hosts_for_events, events_and_context + ).addErrback(log_failure, "cache_joined_hosts_for_event failed"), + ) result, _ = await make_deferred_yieldable( gather_results( - ( - run_in_background( - self._persist_events, - requester=requester, - events_and_context=events_and_context, - ratelimit=ratelimit, - extra_users=extra_users, - ), - run_in_background( - self.cache_joined_hosts_for_events, events_and_context - ).addErrback(log_failure, "cache_joined_hosts_for_event failed"), - ), + dl, consumeErrors=True, ) ).addErrback(unwrapFirstError) From 91602deabe3990e8eb867dd945cc2f798aa3fb1a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 16 Aug 2023 13:50:10 +0100 Subject: [PATCH 3/7] Fix annotations in tests.util.test_async_helpers --- tests/util/test_async_helpers.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/util/test_async_helpers.py b/tests/util/test_async_helpers.py index 91cac9822af4..05983ed434b1 100644 --- a/tests/util/test_async_helpers.py +++ b/tests/util/test_async_helpers.py @@ -60,11 +60,9 @@ def check_called_first(res: int) -> int: observer1.addBoth(check_called_first) # store the results - results: List[Optional[ObservableDeferred[int]]] = [None, None] + results: List[Optional[int]] = [None, None] - def check_val( - res: ObservableDeferred[int], idx: int - ) -> ObservableDeferred[int]: + def check_val(res: int, idx: int) -> int: results[idx] = res return res @@ -93,14 +91,14 @@ def check_called_first(res: int) -> int: observer1.addBoth(check_called_first) # store the results - results: List[Optional[ObservableDeferred[str]]] = [None, None] + results: List[Optional[Failure]] = [None, None] - def check_val(res: ObservableDeferred[str], idx: int) -> None: + def check_failure(res: Failure, idx: int) -> None: results[idx] = res return None - observer1.addErrback(check_val, 0) - observer2.addErrback(check_val, 1) + observer1.addErrback(check_failure, 0) + observer2.addErrback(check_failure, 1) try: raise Exception("gah!") From 78a94d5e742ec2dbcf2f2299cafe3ba21292aa1d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 16 Aug 2023 13:57:37 +0100 Subject: [PATCH 4/7] Make synapse.util.caches.deferred_cache typecheck --- synapse/util/caches/deferred_cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/util/caches/deferred_cache.py b/synapse/util/caches/deferred_cache.py index bf7bd351e0cc..029eedcc6fae 100644 --- a/synapse/util/caches/deferred_cache.py +++ b/synapse/util/caches/deferred_cache.py @@ -470,7 +470,7 @@ def __init__(self) -> None: def deferred(self, key: KT) -> "defer.Deferred[VT]": if not self._deferred: self._deferred = ObservableDeferred(defer.Deferred(), consumeErrors=True) - return self._deferred.observe().addCallback(lambda res: res.get(key)) + return self._deferred.observe().addCallback(lambda res: res[key]) def add_invalidation_callback( self, key: KT, callback: Optional[Callable[[], None]] From 2dd794cc34e26db878ce34865c03241fe9543c97 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 16 Aug 2023 14:02:04 +0100 Subject: [PATCH 5/7] Don't complain about reduntant casts for twisted trunk --- .github/workflows/twisted_trunk.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/twisted_trunk.yml b/.github/workflows/twisted_trunk.yml index 67ccc03f6e2d..7d629a4ed097 100644 --- a/.github/workflows/twisted_trunk.yml +++ b/.github/workflows/twisted_trunk.yml @@ -54,8 +54,8 @@ jobs: poetry remove twisted poetry add --extras tls git+https://github.com/twisted/twisted.git#${{ inputs.twisted_ref || 'trunk' }} poetry install --no-interaction --extras "all test" - - name: Remove warn_unused_ignores from mypy config - run: sed '/warn_unused_ignores = True/d' -i mypy.ini + - name: Remove unhelpful options from mypy config + run: sed -e '/warn_unused_ignores = True/d' -e '/warn_redundant_casts = True/d' -i mypy.ini - run: poetry run mypy trial: From 95b0dabd443795c4f78d110b2b4fd70996e55c5e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 16 Aug 2023 14:58:47 +0100 Subject: [PATCH 6/7] Changelog --- changelog.d/16121.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16121.misc diff --git a/changelog.d/16121.misc b/changelog.d/16121.misc new file mode 100644 index 000000000000..f325d2a31dbd --- /dev/null +++ b/changelog.d/16121.misc @@ -0,0 +1 @@ +Attempt to fix the twisted trunk job. From 0b63e34d0e7be76fd5c0c1e850caea68a0cb5d19 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 24 Aug 2023 10:01:34 -0400 Subject: [PATCH 7/7] Review comments. --- synapse/handlers/message.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 8a65bb648a10..c4ca5065d884 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1474,9 +1474,10 @@ async def handle_new_client_event( # We now persist the event (and update the cache in parallel, since we # don't want to block on it). + # # Note: mypy gets confused if we inline dl and check with twisted#11770. # Some kind of bug in mypy's deduction? - dl = ( + deferreds = ( run_in_background( self._persist_events, requester=requester, @@ -1489,10 +1490,7 @@ async def handle_new_client_event( ).addErrback(log_failure, "cache_joined_hosts_for_event failed"), ) result, _ = await make_deferred_yieldable( - gather_results( - dl, - consumeErrors=True, - ) + gather_results(deferreds, consumeErrors=True) ).addErrback(unwrapFirstError) return result