Fix some comments and types in service notices #7996

clokep · 2020-07-30T15:16:20Z

A bunch of synapse.server_notices still referred to Deferreds when they were already async.

This also adds some type info and does a few other things I'll call out in comments.

synapse/server_notices/consent_server_notices.py

synapse/server_notices/server_notices_manager.py

anoadragon453 · 2020-07-31T04:17:21Z

synapse/server_notices/resource_limits_server_notices.py

@@ -135,7 +135,7 @@ async def _remove_limit_block_notification(
        )

    async def _apply_limit_block_notification(
-        self, user_id: str, event_body: str, event_limit_type: str
+        self, user_id: str, event_body: str, event_limit_type: Optional[str]


I'm not sure if I see much evidence of event_limit_type ever being set to None by the calling function?

synapse/synapse/server_notices/resource_limits_server_notices.py

Lines 88 to 97 in 4a3f15d

limit_msg = None

limit_type = None

try:

# Normally should always pass in user_id to check_auth_blocking

# if you have it, but in this case are checking what would happen

# to other users if they were to arrive.

await self._auth.check_auth_blocking()

except ResourceLimitError as e:

limit_msg = e.msg

limit_type = e.limit_type

limit_type only gets set on error.

(I should also note that mypy found this.)

It looks like in the linked code block that _apply_limit_block_notification will only be called if limit_msg is not None, and as limit_msg and limit_type are both set together, _apply_limit_block_notification should only be called when limit_type is not None?

I suppose that may just be circumstantial of this one calling function though, and _apply_limit_block_notification can handle limit_type=None just fine, and thus that's what the signature should be. I found it a bit difficult to verify that last bit though...

Ah, I see what you're saying. Let me see if I can convince mypy here...

Yeah so mypy doesn't seem to recognize that these are both set or neither is set. I switched it around a bit in 052a474, but ended up ignoring that function call. I believe the correct signature is str, not Optional[str], so I'd rather the signature be correct.

Let me know if you agree!

anoadragon453

@clokep Github doesn't want me to comment on the original thread, so I'll say here that ignoring works and seem fine, though we may want to leave a small comment explaining why we did that.

synapse/server_notices/server_notices_manager.py

@lugino-emeritus

Synapse 1.19.0rc1 (2020-08-13) ============================== Removal warning --------------- As outlined in the [previous release](https://github.com/matrix-org/synapse/releases/tag/v1.18.0), we are no longer publishing Docker images with the `-py3` tag suffix. On top of that, we have also removed the `latest-py3` tag. Please see [the announcement in the upgrade notes for 1.18.0](https://github.com/matrix-org/synapse/blob/develop/UPGRADE.rst#upgrading-to-v1180). Features -------- - Add option to allow server admins to join rooms which fail complexity checks. Contributed by @lugino-emeritus. ([\#7902](#7902)) - Add an option to purge room or not with delete room admin endpoint (`POST /_synapse/admin/v1/rooms/<room_id>/delete`). Contributed by @dklimpel. ([\#7964](#7964)) - Add rate limiting to users joining rooms. ([\#8008](#8008)) - Add a `/health` endpoint to every configured HTTP listener that can be used as a health check endpoint by load balancers. ([\#8048](#8048)) - Allow login to be blocked based on the values of SAML attributes. ([\#8052](#8052)) - Allow guest access to the `GET /_matrix/client/r0/rooms/{room_id}/members` endpoint, according to MSC2689. Contributed by Awesome Technologies Innovationslabor GmbH. ([\#7314](#7314)) Bugfixes -------- - Fix a bug introduced in Synapse v1.7.2 which caused inaccurate membership counts in the room directory. ([\#7977](#7977)) - Fix a long standing bug: 'Duplicate key value violates unique constraint "event_relations_id"' when message retention is configured. ([\#7978](#7978)) - Fix "no create event in auth events" when trying to reject invitation after inviter leaves. Bug introduced in Synapse v1.10.0. ([\#7980](#7980)) - Fix various comments and minor discrepencies in server notices code. ([\#7996](#7996)) - Fix a long standing bug where HTTP HEAD requests resulted in a 400 error. ([\#7999](#7999)) - Fix a long-standing bug which caused two copies of some log lines to be written when synctl was used along with a MemoryHandler logger. ([\#8011](#8011), [\#8012](#8012)) Updates to the Docker image --------------------------- - We no longer publish Docker images with the `-py3` tag suffix, as [announced in the upgrade notes](https://github.com/matrix-org/synapse/blob/develop/UPGRADE.rst#upgrading-to-v1180). ([\#8056](#8056)) Improved Documentation ---------------------- - Document how to set up a client .well-known file and fix several pieces of outdated documentation. ([\#7899](#7899)) - Improve workers docs. ([\#7990](#7990), [\#8000](#8000)) - Fix typo in `docs/workers.md`. ([\#7992](#7992)) - Add documentation for how to undo a room shutdown. ([\#7998](#7998), [\#8010](#8010)) Internal Changes ---------------- - Reduce the amount of whitespace in JSON stored and sent in responses. Contributed by David Vo. ([\#7372](#7372)) - Switch to the JSON implementation from the standard library and bump the minimum version of the canonicaljson library to 1.2.0. ([\#7936](#7936), [\#7979](#7979)) - Convert various parts of the codebase to async/await. ([\#7947](#7947), [\#7948](#7948), [\#7949](#7949), [\#7951](#7951), [\#7963](#7963), [\#7973](#7973), [\#7975](#7975), [\#7976](#7976), [\#7981](#7981), [\#7987](#7987), [\#7989](#7989), [\#8003](#8003), [\#8014](#8014), [\#8016](#8016), [\#8027](#8027), [\#8031](#8031), [\#8032](#8032), [\#8035](#8035), [\#8042](#8042), [\#8044](#8044), [\#8045](#8045), [\#8061](#8061), [\#8062](#8062), [\#8063](#8063), [\#8066](#8066), [\#8069](#8069), [\#8070](#8070)) - Move some database-related log lines from the default logger to the database/transaction loggers. ([\#7952](#7952)) - Add a script to detect source code files using non-unix line terminators. ([\#7965](#7965), [\#7970](#7970)) - Log the SAML session ID during creation. ([\#7971](#7971)) - Implement new experimental push rules for some users. ([\#7997](#7997)) - Remove redundant and unreliable signature check for v1 Identity Service lookup responses. ([\#8001](#8001)) - Improve the performance of the register endpoint. ([\#8009](#8009)) - Reduce less useful output in the newsfragment CI step. Add a link to the changelog section of the contributing guide on error. ([\#8024](#8024)) - Rename storage layer objects to be more sensible. ([\#8033](#8033)) - Change the default log config to reduce disk I/O and storage for new servers. ([\#8040](#8040)) - Add an assertion on `prev_events` in `create_new_client_event`. ([\#8041](#8041)) - Add a comment to `ServerContextFactory` about the use of `SSLv23_METHOD`. ([\#8043](#8043)) - Log `OPTIONS` requests at `DEBUG` rather than `INFO` level to reduce amount logged at `INFO`. ([\#8049](#8049)) - Reduce amount of outbound request logging at `INFO` level. ([\#8050](#8050)) - It is no longer necessary to explicitly define `filters` in the logging configuration. (Continuing to do so is redundant but harmless.) ([\#8051](#8051)) - Add and improve type hints. ([\#8058](#8058), [\#8064](#8064), [\#8060](#8060), [\#8067](#8067))

* commit 'd1008fe94': Fix some comments and types in service notices (#7996)

clokep added 3 commits July 30, 2020 11:05

Stop references Deferreds in comments.

dd1fbb2

Add to tox and fix a few more issues.

4a3f15d

Add changelog.

a9d97d8

clokep requested a review from a team July 30, 2020 17:01

clokep marked this pull request as ready for review July 30, 2020 17:01

clokep mentioned this pull request Jul 30, 2020

Convert inlineCallbacks to async/await #7988

Closed

48 tasks

anoadragon453 reviewed Jul 31, 2020

View reviewed changes

Add more typing info.

699eb37

clokep requested a review from anoadragon453 July 31, 2020 11:20

Ignore mypy on a line.

052a474

anoadragon453 approved these changes Jul 31, 2020

View reviewed changes

synapse/server_notices/server_notices_manager.py Show resolved Hide resolved

clokep merged commit d1008fe into develop Jul 31, 2020

clokep deleted the clokep/async-server-notices branch July 31, 2020 20:22

babolivier pushed a commit that referenced this pull request Sep 1, 2021

Merge commit 'd1008fe94' into anoa/dinsic_release_1_21_x

ab298b0

* commit 'd1008fe94': Fix some comments and types in service notices (#7996)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some comments and types in service notices #7996

Fix some comments and types in service notices #7996

clokep commented Jul 30, 2020

anoadragon453 Jul 31, 2020

clokep Jul 31, 2020

clokep Jul 31, 2020

anoadragon453 Jul 31, 2020

clokep Jul 31, 2020

clokep Jul 31, 2020

anoadragon453 left a comment

	limit_msg = None
	limit_type = None
	try:
	# Normally should always pass in user_id to check_auth_blocking
	# if you have it, but in this case are checking what would happen
	# to other users if they were to arrive.
	await self._auth.check_auth_blocking()
	except ResourceLimitError as e:
	limit_msg = e.msg
	limit_type = e.limit_type

Fix some comments and types in service notices #7996

Fix some comments and types in service notices #7996

Conversation

clokep commented Jul 30, 2020

anoadragon453 Jul 31, 2020

Choose a reason for hiding this comment

clokep Jul 31, 2020

Choose a reason for hiding this comment

clokep Jul 31, 2020

Choose a reason for hiding this comment

anoadragon453 Jul 31, 2020

Choose a reason for hiding this comment

clokep Jul 31, 2020

Choose a reason for hiding this comment

clokep Jul 31, 2020

Choose a reason for hiding this comment

anoadragon453 left a comment

Choose a reason for hiding this comment