Fix typechecking with twisted trunk #16121

DMRobertson · 2023-08-16T13:48:37Z

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.

DMRobertson · 2023-08-16T14:03:26Z

Triggered twisted trunk against 95b0dab: https://github.com/matrix-org/synapse/actions/runs/5879802834

... which was green!

DMRobertson · 2023-08-16T15:19:59Z

tests/util/test_async_helpers.py

-        def check_val(
-            res: ObservableDeferred[int], idx: int
-        ) -> ObservableDeferred[int]:
+        def check_val(res: int, idx: int) -> int:


I checked this was correct by running the test with print(type(res)) inserted.

DMRobertson · 2023-08-16T15:20:32Z

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])


I think the point is that this lookup never fails---but please double check.

I'm very not-confident about this change. I think you're correct, but I'm very lost in how this is used.

Alright. I took another look at this and I agree that if this is not true then the internal machinery is broken. It'll raise errors anyway, so I think this is OK. (Otherwise you'd get back an unexpected None, which seems worse TBH)

clokep

A couple of thoughts, but overall seems good.

synapse/handlers/message.py

clokep · 2023-08-21T12:36:42Z

synapse/handlers/message.py

-        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 = (


I don't get what dl stands for, can we use a longer variable name?

I think it is "deferred list"?

Yes, deferred list.

Thanks for getting this over the line.

clokep · 2023-08-21T12:37:10Z

synapse/handlers/message.py

            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,
            )


Maybe remove the comma after consumerErrors so black folds this a bit tighter?

clokep · 2023-08-21T12:39:43Z

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])


I'm very not-confident about this change. I think you're correct, but I'm very lost in how this is used.

…mypy

@evilham

- Add configuration setting for CAS protocol version. Contributed by Aurélien Grimpard. ([\#15816](#15816)) - Suppress notifications from message edits per [MSC3958](matrix-org/matrix-spec-proposals#3958). ([\#16113](#16113)) - Return a `Retry-After` with `M_LIMIT_EXCEEDED` error responses. ([\#16136](#16136)) - Add `last_seen_ts` to the [admin users API](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html). ([\#16218](#16218)) - Improve resource usage when sending data to a large number of remote hosts that are marked as "down". ([\#16223](#16223)) - Fix IPv6-related bugs on SMTP settings, adding groundwork to fix similar issues. Contributed by @evilham and @telmich (ungleich.ch). ([\#16155](#16155)) - Fix a spec compliance issue where requests to the `/publicRooms` federation API would specify `include_all_networks` as a string. ([\#16185](#16185)) - Fix inaccurate error message while attempting to ban or unban a user with the same or higher PL by spliting the conditional statements. Contributed by @leviosacz. ([\#16205](#16205)) - Fix a rare bug that broke looping calls, which could lead to e.g. linearly increasing memory usage. Introduced in v1.90.0. ([\#16210](#16210)) - Fix a long-standing bug where uploading images would fail if we could not generate thumbnails for them. ([\#16211](#16211)) - Fix a long-standing bug where we did not correctly back off from servers that had "gone" if they returned 4xx series error codes. ([\#16221](#16221)) - Update links to the [matrix.org blog](https://matrix.org/blog/). ([\#16008](#16008)) - Document which [admin APIs](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html) are disabled when experimental [MSC3861](matrix-org/matrix-spec-proposals#3861) support is enabled. ([\#16168](#16168)) - Document [`exclude_rooms_from_sync`](https://matrix-org.github.io/synapse/v1.92/usage/configuration/config_documentation.html#exclude_rooms_from_sync) configuration option. ([\#16178](#16178)) - Prepare unit tests for Python 3.12. ([\#16099](#16099)) - Fix nightly CI jobs. ([\#16121](#16121), [\#16213](#16213)) - Describe which rate limiter was hit in logs. ([\#16135](#16135)) - Simplify presence code when using workers. ([\#16170](#16170)) - Track per-device information in the presence code. ([\#16171](#16171), [\#16172](#16172)) - Stop using the `event_txn_id` table. ([\#16175](#16175)) - Use `AsyncMock` instead of custom code. ([\#16179](#16179), [\#16180](#16180)) - Improve error reporting of invalid data passed to `/_matrix/key/v2/query`. ([\#16183](#16183)) - Task scheduler: add replication notify for new task to launch ASAP. ([\#16184](#16184)) - Improve type hints. ([\#16186](#16186), [\#16188](#16188), [\#16201](#16201)) - Bump black version to 23.7.0. ([\#16187](#16187)) - Log the details of background update failures. ([\#16212](#16212)) - Cache device resync requests over replication. ([\#16241](#16241)) * Bump anyhow from 1.0.72 to 1.0.75. ([\#16141](#16141)) * Bump furo from 2023.7.26 to 2023.8.19. ([\#16238](#16238)) * Bump phonenumbers from 8.13.18 to 8.13.19. ([\#16237](#16237)) * Bump psycopg2 from 2.9.6 to 2.9.7. ([\#16196](#16196)) * Bump regex from 1.9.3 to 1.9.4. ([\#16195](#16195)) * Bump ruff from 0.0.277 to 0.0.286. ([\#16198](#16198)) * Bump sentry-sdk from 1.29.2 to 1.30.0. ([\#16236](#16236)) * Bump serde from 1.0.184 to 1.0.188. ([\#16194](#16194)) * Bump serde_json from 1.0.104 to 1.0.105. ([\#16140](#16140)) * Bump types-psycopg2 from 2.9.21.10 to 2.9.21.11. ([\#16200](#16200)) * Bump types-pyyaml from 6.0.12.10 to 6.0.12.11. ([\#16199](#16199))

David Robertson added 6 commits August 16, 2023 12:47

Make synapse.logging.context lint w/ twisted trunk

f880e80

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.

Make synapse.handlers.message work w/ twisted trunk

de25f6a

Fix annotations in tests.util.test_async_helpers

91602de

Make synapse.util.caches.deferred_cache typecheck

78a94d5

Don't complain about reduntant casts for twisted trunk

2dd794c

Changelog

95b0dab

DMRobertson force-pushed the dmr/twisted-trunk-mypy branch from 3aa4c17 to 95b0dab Compare August 16, 2023 14:00

DMRobertson marked this pull request as ready for review August 16, 2023 15:19

DMRobertson requested a review from a team as a code owner August 16, 2023 15:19

DMRobertson commented Aug 16, 2023

View reviewed changes

clokep reviewed Aug 21, 2023

View reviewed changes

clokep added 2 commits August 24, 2023 10:01

Review comments.

0b63e34

Merge remote-tracking branch 'origin/develop' into dmr/twisted-trunk-…

22dfa69

…mypy

clokep approved these changes Aug 24, 2023

View reviewed changes

clokep enabled auto-merge (squash) August 24, 2023 14:07

clokep merged commit e691243 into develop Aug 24, 2023
37 checks passed

clokep deleted the dmr/twisted-trunk-mypy branch August 24, 2023 14:53

DMRobertson mentioned this pull request Aug 30, 2023

CI run against latest deps is failing #16100

Closed

hughns pushed a commit that referenced this pull request Sep 4, 2023

Fix typechecking with twisted trunk (#16121)

af6f799

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typechecking with twisted trunk #16121

Fix typechecking with twisted trunk #16121

DMRobertson commented Aug 16, 2023

DMRobertson commented Aug 16, 2023 •

edited

Loading

DMRobertson Aug 16, 2023

DMRobertson Aug 16, 2023

clokep Aug 21, 2023

clokep Aug 24, 2023

clokep left a comment

clokep Aug 21, 2023

clokep Aug 24, 2023

DMRobertson Aug 28, 2023

clokep Aug 21, 2023

clokep Aug 21, 2023

Fix typechecking with twisted trunk #16121

Fix typechecking with twisted trunk #16121

Conversation

DMRobertson commented Aug 16, 2023

DMRobertson commented Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMRobertson commented Aug 16, 2023 •

edited

Loading