server notices on resource limit blocking #3680
Conversation
…rver_notices_on_blocking
…rver_notices_on_blocking
…rver_notices_on_blocking
…rver_notices_on_blocking
neilisfragile
changed the title
|
Tests needs some love, but probably good enough to get going. |
| @@ -103,7 +103,7 @@ def maybe_send_server_notice_to_user(self, user_id): | |||
| }, | |||
| ) | |||
| yield self._server_notices_manager.send_notice( | |||
| user_id, content, | |||
| user_id, content | |||
There was a problem hiding this comment.
This is a spurious change, and in general we want to have trailing commas
synapse/api/constants.py
Outdated
| @@ -78,6 +78,9 @@ class EventTypes(object): | |||
| Name = "m.room.name" | |||
|
|
|||
| ServerACL = "m.room.server_acl" | |||
| Pinned = "m.room.pinned_events" | |||
|
|
|||
| ServerNoticeLimitReached = "m.server_notice.usage_limit_reached" | |||
There was a problem hiding this comment.
Is m.server_notice.* already a thing?
There was a problem hiding this comment.
|
|
||
| class ResourceLimitsServerNotices(object): | ||
| """ | ||
| """ |
There was a problem hiding this comment.
Some documentation in the docstrings would be fab.
| self._server_notice_content = hs.config.user_consent_server_notice_content | ||
| self._admin_uri = hs.config.admin_uri | ||
| self._limit_usage_by_mau = hs.config.limit_usage_by_mau | ||
| self._hs_disabled = hs.config.hs_disabled |
There was a problem hiding this comment.
Usually we just have self.config. It doesn't really matter, but makes it easier when writing unit tests where you can then cheekily change the config and have it updated everywhere where necessary.
| self._resouce_limited = False | ||
| self._message_handler = hs.get_message_handler() | ||
| self._state = hs.get_state_handler() | ||
| # Config checks? |
There was a problem hiding this comment.
Should this be here? Lets include the checks now if we should have them
There was a problem hiding this comment.
irl discussion, noop
| # Normally should always pass in user_id if you have it, but in | ||
| # this case are checking what would happen to other users if they | ||
| # were to arrive. | ||
| yield self.auth.check_auth_blocking() |
There was a problem hiding this comment.
This feels like a bit of an awful thing to do, but lets keep it like this for now, but write is as:
try:
yield self.auth.check_auth_blocking()
is_auth_blocking = False
except AuthError as e:
is_auth_blocking = TrueRather than having nested exception, which can get weird quickly
There was a problem hiding this comment.
fair enough both branches started off very neat, much prefer suggested approach
| user_id, content, EventTypes.Pinned, '', | ||
| ) | ||
|
|
||
| except AuthError as e: |
There was a problem hiding this comment.
Aren't we renaming this to ResourceLimitError or whatever it was?
There was a problem hiding this comment.
PR racing :-)
| referenced_events = pinned_state_event.content.get('pinned') | ||
|
|
||
| events = yield self._store.get_events(referenced_events) | ||
| for event_id, event in events.items(): |
There was a problem hiding this comment.
We should always use iteritems(events), to ensure that we don't needlessly copy in py2
There was a problem hiding this comment.
I did it this way for py3 compat. best I can tell iteritems is not supported in py3
There was a problem hiding this comment.
There's from six import iteritems, which does it
| } | ||
| event = yield self._server_notices_manager.send_notice( | ||
| user_id, content, EventTypes.ServerNoticeLimitReached | ||
| ) |
There was a problem hiding this comment.
Err, have we actually finally decided that clients will render anything with body rather than just those with type m.room.message?
There was a problem hiding this comment.
Not sure, am following spec proposal - is there a broader conversation to be had here?
There was a problem hiding this comment.
|
In general, I'm worried that this is going to cause quite a lot of slowness on servers where this is enabled; doing this check on every sync feels excessive. I'm also unsure we only want to send the notice when a user comes back online again. I think the correct time we'd want to do all this is when we do the transition from being open to closed, and vice versa. |
…rver_notices_on_blocking
…rver_notices_on_blocking
|
Agreed that okay to ship with perf concerns, though will refactor (rsn) to store the current room state into something cacheable |
|
Fixing bug that prevented the server notices user sending events to the client |
|
I'm currently waiting for the resolution of matrix-org/matrix-spec-proposals#1452 before merging |
…rver_notices_on_blocking
|
Looks like the CI failure is a flakey presence sytest |
WIP implementation, need to clarify on the shape of the event itself.