Improve `RawHeaders` type hints #14303

DMRobertson · 2022-10-26T13:11:01Z

Follow up to #14301. Needs that to be merged into develop.

This would have detected the bug that that PR fixed.

DMRobertson · 2022-10-26T13:15:29Z

synapse/app/generic_worker.py

-            headers = {
-                header: request.requestHeaders.getRawHeaders(header, [])
+            headers: Dict[bytes, List[bytes]] = {
+                header: list(request.requestHeaders.getRawHeaders(header, []))


AFAICS getRawHeaders always returns a list, so I think this is an unfortunate pointless copy. It's needed because Twisted's annotations only promise it returns a Sequence.

IMO this is a bug in Twisted's type hints: as I understand it, the rule of thumb is that functions ought to be liberal in what they accept but precise in what they return.

I mean, that rule certainly makes sense but it's also kind of fair enough to give minimal guarantees :/.

Should we use a tuple instead? My understanding is that lists usually have extra allocated space so that you can extend them, but presumably we're not interested in growing these and we'll treat them as immutable anyway.

I mean, that rule certainly makes sense but it's also kind of fair enough to give minimal guarantees :/.

I sympathise. I suppose there are different trade-offs to be made at the library and at the application level.

Shout-out to Hyrum's Law while we're at it.

Should we use a tuple instead? My understanding is that lists usually have extra allocated space so that you can extend them, but presumably we're not interested in growing these and we'll treat them as immutable anyway.

No strong opinions. I doubt it makes a perceivable difference: this isn't a hot path, and in either case the interpreter will have to alloc a new container + underlying storage and bump the refcounts for every element.

DMRobertson · 2022-10-26T13:20:33Z

This is going to fail until #14301 lands on develop. (But that at least demonstrates that the type hints do detect the problem.)

to detect the problem that #14301 fixed.

reivilibre · 2022-10-26T13:36:32Z

synapse/app/generic_worker.py

-            headers = {
-                header: request.requestHeaders.getRawHeaders(header, [])
+            headers: Dict[bytes, List[bytes]] = {
+                header: list(request.requestHeaders.getRawHeaders(header, []))


I mean, that rule certainly makes sense but it's also kind of fair enough to give minimal guarantees :/.

reivilibre · 2022-10-26T13:37:45Z

synapse/app/generic_worker.py

-            headers = {
-                header: request.requestHeaders.getRawHeaders(header, [])
+            headers: Dict[bytes, List[bytes]] = {
+                header: list(request.requestHeaders.getRawHeaders(header, []))


Should we use a tuple instead? My understanding is that lists usually have extra allocated space so that you can extend them, but presumably we're not interested in growing these and we'll treat them as immutable anyway.

@asymmetric

Synapse 1.71.0 (2022-11-08) =========================== Please note that, as announced in the release notes for Synapse 1.69.0, legacy Prometheus metric names are now disabled by default. They will be removed altogether in Synapse 1.73.0. If not already done, server administrators should update their dashboards and alerting rules to avoid using the deprecated metric names. See the [upgrade notes](https://matrix-org.github.io/synapse/v1.71/upgrade.html#upgrading-to-v1710) for more details. **Note:** in line with our [deprecation policy](https://matrix-org.github.io/synapse/latest/deprecation_policy.html) for platform dependencies, this will be the last release to support PostgreSQL 10, which reaches upstream end-of-life on November 10th, 2022. Future releases of Synapse will require PostgreSQL 11+. No significant changes since 1.71.0rc2. Synapse 1.71.0rc2 (2022-11-04) ============================== Improved Documentation ---------------------- - Document the changes to monthly active user metrics due to deprecation of legacy Prometheus metric names. ([\matrix-org#14358](matrix-org#14358), [\matrix-org#14360](matrix-org#14360)) Deprecations and Removals ------------------------- - Disable legacy Prometheus metric names by default. They can still be re-enabled for now, but they will be removed altogether in Synapse 1.73.0. ([\matrix-org#14353](matrix-org#14353)) Internal Changes ---------------- - Run unit tests against Python 3.11. ([\matrix-org#13812](matrix-org#13812)) Synapse 1.71.0rc1 (2022-11-01) ============================== Features -------- - Support back-channel logouts from OpenID Connect providers. ([\matrix-org#11414](matrix-org#11414)) - Allow use of Postgres and SQLlite full-text search operators in search queries. ([\matrix-org#11635](matrix-org#11635), [\matrix-org#14310](matrix-org#14310), [\matrix-org#14311](matrix-org#14311)) - Implement [MSC3664](matrix-org/matrix-spec-proposals#3664), Pushrules for relations. Contributed by Nico. ([\matrix-org#11804](matrix-org#11804)) - Improve aesthetics of HTML templates. Note that these changes do not retroactively apply to templates which have been [customised](https://matrix-org.github.io/synapse/latest/templates.html#templates) by server admins. ([\matrix-org#13652](matrix-org#13652)) - Enable write-ahead logging for SQLite installations. Contributed by [@asymmetric](https://github.com/asymmetric). ([\matrix-org#13897](matrix-org#13897)) - Show erasure status when [listing users](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#query-user-account) in the Admin API. ([\matrix-org#14205](matrix-org#14205)) - Provide a specific error code when a `/sync` request provides a filter which doesn't represent a JSON object. ([\matrix-org#14262](matrix-org#14262)) Bugfixes -------- - Fix a long-standing bug where the `update_synapse_database` script could not be run with multiple databases. Contributed by @thefinn93 @ Beeper. ([\matrix-org#13422](matrix-org#13422)) - Fix a bug which prevented setting an avatar on homeservers which have an explicit port in their `server_name` and have `max_avatar_size` and/or `allowed_avatar_mimetypes` configuration. Contributed by @ashfame. ([\matrix-org#13927](matrix-org#13927)) - Check appservice user interest against the local users instead of all users in the room to align with [MSC3905](matrix-org/matrix-spec-proposals#3905). ([\matrix-org#13958](matrix-org#13958)) - Fix a long-standing bug where Synapse would accidentally include extra information in the response to [`PUT /_matrix/federation/v2/invite/{roomId}/{eventId}`](https://spec.matrix.org/v1.4/server-server-api/#put_matrixfederationv2inviteroomideventid). ([\matrix-org#14064](matrix-org#14064)) - Fix a bug introduced in Synapse 1.64.0 where presence updates could be missing from `/sync` responses. ([\matrix-org#14243](matrix-org#14243)) - Fix a bug introduced in Synapse 1.60.0 which caused an error to be logged when Synapse received a SIGHUP signal if debug logging was enabled. ([\matrix-org#14258](matrix-org#14258)) - Prevent history insertion ([MSC2716](matrix-org/matrix-spec-proposals#2716)) during an partial join ([MSC3706](matrix-org/matrix-spec-proposals#3706)). ([\matrix-org#14291](matrix-org#14291)) - Fix a bug introduced in Synapse 1.34.0 where device names would be returned via a federation user key query request when `allow_device_name_lookup_over_federation` was set to `false`. ([\matrix-org#14304](matrix-org#14304)) - Fix a bug introduced in Synapse 0.34.0 where logs could include error spam when background processes are measured as taking a negative amount of time. ([\matrix-org#14323](matrix-org#14323)) - Fix a bug introduced in Synapse 1.70.0 where clients were unable to PUT new [dehydrated devices](matrix-org/matrix-spec-proposals#2697). ([\matrix-org#14336](matrix-org#14336)) Improved Documentation ---------------------- - Explain how to disable the use of [`trusted_key_servers`](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#trusted_key_servers). ([\matrix-org#13999](matrix-org#13999)) - Add workers settings to [configuration manual](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#individual-worker-configuration). ([\matrix-org#14086](matrix-org#14086)) - Correct the name of the config option [`encryption_enabled_by_default_for_room_type`](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#encryption_enabled_by_default_for_room_type). ([\matrix-org#14110](matrix-org#14110)) - Update docstrings of `SynapseError` and `FederationError` to bettter describe what they are used for and the effects of using them are. ([\matrix-org#14191](matrix-org#14191)) Internal Changes ---------------- - Remove unused `@lru_cache` decorator. ([\matrix-org#13595](matrix-org#13595)) - Save login tokens in database and prevent login token reuse. ([\matrix-org#13844](matrix-org#13844)) - Refactor OIDC tests to better mimic an actual OIDC provider. ([\matrix-org#13910](matrix-org#13910)) - Fix type annotation causing import time error in the Complement forking launcher. ([\matrix-org#14084](matrix-org#14084)) - Refactor [MSC3030](matrix-org/matrix-spec-proposals#3030) `/timestamp_to_event` endpoint to loop over federation destinations with standard pattern and error handling. ([\matrix-org#14096](matrix-org#14096)) - Add initial power level event to batch of bulk persisted events when creating a new room. ([\matrix-org#14228](matrix-org#14228)) - Refactor `/key/` endpoints to use `RestServlet` classes. ([\matrix-org#14229](matrix-org#14229)) - Switch to using the `matrix-org/backend-meta` version of `triage-incoming` for new issues in CI. ([\matrix-org#14230](matrix-org#14230)) - Build wheels on macos 11, not 10.15. ([\matrix-org#14249](matrix-org#14249)) - Add debugging to help diagnose lost device list updates. ([\matrix-org#14268](matrix-org#14268)) - Add Rust cache to CI for `trial` runs. ([\matrix-org#14287](matrix-org#14287)) - Improve type hinting of `RawHeaders`. ([\matrix-org#14303](matrix-org#14303)) - Use Poetry 1.2.0 in the Twisted Trunk CI job. ([\matrix-org#14305](matrix-org#14305)) <details> <summary>Dependency updates</summary> Runtime: - Bump anyhow from 1.0.65 to 1.0.66. ([\matrix-org#14278](matrix-org#14278)) - Bump jinja2 from 3.0.3 to 3.1.2. ([\matrix-org#14271](matrix-org#14271)) - Bump prometheus-client from 0.14.0 to 0.15.0. ([\matrix-org#14274](matrix-org#14274)) - Bump psycopg2 from 2.9.4 to 2.9.5. ([\matrix-org#14331](matrix-org#14331)) - Bump pysaml2 from 7.1.2 to 7.2.1. ([\matrix-org#14270](matrix-org#14270)) - Bump sentry-sdk from 1.5.11 to 1.10.1. ([\matrix-org#14330](matrix-org#14330)) - Bump serde from 1.0.145 to 1.0.147. ([\matrix-org#14277](matrix-org#14277)) - Bump serde_json from 1.0.86 to 1.0.87. ([\matrix-org#14279](matrix-org#14279)) Tooling and CI: - Bump black from 22.3.0 to 22.10.0. ([\matrix-org#14328](matrix-org#14328)) - Bump flake8-bugbear from 21.3.2 to 22.9.23. ([\matrix-org#14042](matrix-org#14042)) - Bump peaceiris/actions-gh-pages from 3.8.0 to 3.9.0. ([\matrix-org#14276](matrix-org#14276)) - Bump peaceiris/actions-mdbook from 1.1.14 to 1.2.0. ([\matrix-org#14275](matrix-org#14275)) - Bump setuptools-rust from 1.5.1 to 1.5.2. ([\matrix-org#14273](matrix-org#14273)) - Bump twine from 3.8.0 to 4.0.1. ([\matrix-org#14332](matrix-org#14332)) - Bump types-opentracing from 2.4.7 to 2.4.10. ([\matrix-org#14133](matrix-org#14133)) - Bump types-requests from 2.28.11 to 2.28.11.2. ([\matrix-org#14272](matrix-org#14272)) </details>

DMRobertson force-pushed the dmr/type-hint-headers branch 2 times, most recently from 54d3de8 to 4244b9b Compare October 26, 2022 13:12

DMRobertson commented Oct 26, 2022

View reviewed changes

DMRobertson force-pushed the dmr/type-hint-headers branch from 5f659e5 to 484ebd1 Compare October 26, 2022 13:19

DMRobertson marked this pull request as ready for review October 26, 2022 13:20

DMRobertson requested a review from a team as a code owner October 26, 2022 13:20

David Robertson added 2 commits October 26, 2022 14:25

Improve type hinting of RawHeaders

0f0011a

to detect the problem that #14301 fixed.

Changelog

f5f1df8

DMRobertson force-pushed the dmr/type-hint-headers branch from 484ebd1 to f5f1df8 Compare October 26, 2022 13:25

reivilibre approved these changes Oct 26, 2022

View reviewed changes

David Robertson added 2 commits October 26, 2022 19:00

Merge branch 'develop' into dmr/type-hint-headers

8a4d8cb

Merge branch 'develop' into dmr/type-hint-headers

689b8c3

DMRobertson enabled auto-merge (squash) October 28, 2022 15:59

DMRobertson merged commit 730b13d into develop Oct 28, 2022

DMRobertson deleted the dmr/type-hint-headers branch October 28, 2022 16:04

DMRobertson mentioned this pull request Sep 4, 2023

Send the opentracing span information also to appservices #16227

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `RawHeaders` type hints #14303

Improve `RawHeaders` type hints #14303

DMRobertson commented Oct 26, 2022 •

edited

Loading

DMRobertson Oct 26, 2022

reivilibre Oct 26, 2022

reivilibre Oct 26, 2022

DMRobertson Oct 26, 2022

DMRobertson Oct 26, 2022

DMRobertson commented Oct 26, 2022

reivilibre Oct 26, 2022

reivilibre Oct 26, 2022

Improve RawHeaders type hints #14303

Improve RawHeaders type hints #14303

Conversation

DMRobertson commented Oct 26, 2022 • edited Loading

DMRobertson Oct 26, 2022

Choose a reason for hiding this comment

reivilibre Oct 26, 2022

Choose a reason for hiding this comment

reivilibre Oct 26, 2022

Choose a reason for hiding this comment

DMRobertson Oct 26, 2022

Choose a reason for hiding this comment

DMRobertson Oct 26, 2022

Choose a reason for hiding this comment

DMRobertson commented Oct 26, 2022

reivilibre Oct 26, 2022

Choose a reason for hiding this comment

reivilibre Oct 26, 2022

Choose a reason for hiding this comment

Improve `RawHeaders` type hints #14303

Improve `RawHeaders` type hints #14303

DMRobertson commented Oct 26, 2022 •

edited

Loading