Move high level room joining logic to GMSL #3065
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3065 +/- ##
=======================================
Coverage 66.70% 66.71%
=======================================
Files 495 495
Lines 53559 53473 -86
=======================================
- Hits 35727 35673 -54
+ Misses 14186 14167 -19
+ Partials 3646 3633 -13
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| ) (res gomatrixserverlib.SendJoinResponse, err error) { | ||
| ctx, cancel := context.WithTimeout(ctx, defaultTimeout) | ||
| defer cancel() | ||
| ires, err := a.doRequestIfNotBlacklisted(s, func() (interface{}, error) { |
There was a problem hiding this comment.
I like this but the behaviour in the following situation feels wrong:
- Server X is blacklisted.
- Server X comes back online but Dendrite doesn't know it yet.
- User wants to join a room via Server X.
- Blacklist says no.
- Dendrite marks that as a failure (Reachable=false), reinforcing the blacklist.
I don't know the full implications of this. If reinforcing the blacklist is a no-op then fine, but if it resets the timer or something then I can see this being a problem. The specification is silent on how best to handle blacklists, because after all we don't want to waste time sending requests to dead servers. However, the spec is particularly unclear on which endpoints should be considered "unblacklistable" - i.e always try them to see if the server is now up. We will unblacklist when the remote server sends us something (I hope hope hope) but that may be at some indeterminate point in the future.
The safest option would be to preserve the original implementation and don't check the blacklist. Longer term, we need to take a holistic view of this and figure out a way forward.
GMSL PR: matrix-org/gomatrixserverlib#372