Remove trailing slashes from outbound federation requests by anoadragon453 · Pull Request #4840 · matrix-org/synapse

anoadragon453 · 2019-03-08T16:57:45Z

Client portion of #4793 and second portion of a solution towards #3622

TODO:

Have Synapse send out federation requests without trailing slashes
Have Synapse retry a federation request with a trailing slash if it received a 404
I think the server portion may need to be merged first before CI will be happy here.

codecov · 2019-03-08T18:22:58Z

Codecov Report

Merging #4840 into develop will increase coverage by 0.64%.
The diff coverage is 94.73%.

@@             Coverage Diff             @@
##           develop    #4840      +/-   ##
===========================================
+ Coverage    75.34%   75.99%   +0.64%     
===========================================
  Files          340      327      -13     
  Lines        34924    36941    +2017     
  Branches      5718     6485     +767     
===========================================
+ Hits         26315    28072    +1757     
- Misses        6996     7083      +87     
- Partials      1613     1786     +173

richvdh

yeah. The main problem here is that these endpoints don't actually return a 404 on synapse 0.99.2 if you hit them without a trailing slash. Empirically, they seem to return a 400 with { "errcode": "M_UNRECOGNIZED" }, which is something you could check for -- though I urge you to check that each endpoint actually does that before taking my word for it (again).

richvdh · 2019-03-13T09:05:24Z

                 ignore_backoff=False,
-                 backoff_on_404=False):
+                 backoff_on_404=False,
+                 try_trailing_slash_on_404=False):


Suggested change

try_trailing_slash_on_404=False):

try_trailing_slash_on_404=False,

):

flake unfortunately complains about visual indentations if we do this.

/me shakes fist

richvdh · 2019-03-13T09:13:50Z


            backoff_on_404 (bool): Back off if we get a 404

+            try_trailing_slash_on_404 (bool): True if on a 404 response we


it might be worth adding a note on why this is a useful thing to do ("workaround for #3622 in Synapse 0.99.2 and earlier" ?)

anoadragon453 · 2019-03-13T12:12:43Z

@richvdh Hrm, they seem to all do it. I've left in the check for 404 as well just in case this is new behaviour.

Use 400 method as well as 404. Lint change did not make flake happy.

richvdh

honestly I would be inclined not to do this on 404s. I am like 99% sure that no version of synapse will return a 404, and trying to support 404s in this way complicates things somewhat.

(of course, that also means renaming the options and fixing the comments/docstrings)

richvdh · 2019-03-13T15:28:04Z

+            # Re-enable backoff if enabled
+            send_request_args["backoff_on_404"] = backoff_on_404
+
+            response = yield self.get_json(**send_request_args)


why do we switch to get_json here?

richvdh · 2019-03-13T15:29:14Z

+        # or if a 400 with "M_UNRECOGNIZED" which some endpoints return
+        if (try_trailing_slash_on_404 and
+           (response.code == 404
+            or (response.code == 400


You've tested this, have you?

richvdh · 2019-03-13T15:35:34Z

+
+        # If enabled, retry with a trailing slash if we received a 404
+        if try_trailing_slash_on_404 and response.code == 404:
+            args["path"] += "/"


are you sure this is the right thing to do?

richvdh · 2019-03-13T15:35:36Z

+        response = yield self._send_request(**send_request_args)
+
+        # If enabled, retry with a trailing slash if we received a 404
+        if try_trailing_slash_on_404 and response.code == 404:


you seem to have missed the 400 case here, which brings me to:

how about implementing this with a wrapper which you can use in both cases?

def _send_request_with_optional_trailing_slash(request, try_trailing_slash_on_404=False, **kwargs): self._send_request(request, **kwargs) if that_worked: return fiddle_with(request) self._send_request(request, **kwargs)

Addressed changes

richvdh

it's coming together, I still has suggestions though!

richvdh · 2019-03-20T11:38:44Z

+        request,
+        try_trailing_slash_on_400=False,
+        backoff_on_404=False,
+        send_request_args={},


I'd suggest making this a **kwargs so that you don't need to build a dict in the calling code.

richvdh · 2019-03-20T11:38:59Z

+            try_trailing_slash_on_400 (bool): Whether on receiving a 400
+                'M_UNRECOGNIZED' from the server to retry the request with a
+                trailing slash appended to the request path.
+            404_backoff (bool): Whether to backoff on 404 when making a


it's not called that

richvdh · 2019-03-20T11:39:36Z

+                'M_UNRECOGNIZED' from the server to retry the request with a
+                trailing slash appended to the request path.
+            404_backoff (bool): Whether to backoff on 404 when making a
+                request with a trailing slash (only affects request if


why do we only do this on the retry? surely it is just as applicable for the first one?

Mm, good point. When we originally thought we were going to get a 404 on missing a trailing slash rather than a 400, it made sense to not backoff forever before trying again with a trailing slash.

Now there's no need not to do it for best, which consequently also mades the code simpler.

richvdh · 2019-03-20T11:43:23Z

+            if not try_trailing_slash_on_400:
+                # Received an error >= 300. Raise unless we're retrying
+                raise e
+        except Exception as e:


this is a no-op

richvdh · 2019-03-20T11:43:32Z

+        except HttpResponseException as e:
+            if not try_trailing_slash_on_400:
+                # Received an error >= 300. Raise unless we're retrying
+                raise e


Suggested change

raise e

raise

richvdh · 2019-03-20T11:54:12Z

+            response = yield self._send_request(**send_request_args)
+
+            # Check if it's necessary to retry with a trailing slash
+            body = yield _handle_json_response(


I don't think this (or the other instance need to be in here). The exception, if it happens, will get thrown by _send_request. Suggest having _send_request_with_optional_trailing_slash return the response, and leaving the calls to _handle_json_response where they were.

It will be necessary if we're checking the body for M_UNRECOGNIZED, which we need to do again anyways.

richvdh · 2019-03-20T11:54:14Z

+        try:
+            response = yield self._send_request(**send_request_args)
+
+            # Check if it's necessary to retry with a trailing slash


I'm not sure what this comment means

Addressed changes

richvdh

can you clean up backoff_on_404?

richvdh · 2019-03-21T14:15:13Z

+        self,
+        request,
+        try_trailing_slash_on_400=False,
+        backoff_on_404=False,


I don't think there's any need for this now. You can elide it and just let send_request_args pick it up.

(if there are things that rely on passing it as a positional, rather than keyword, arg, then add a *args or, better, fix the callers to use the arg name)

richvdh · 2019-03-21T14:25:17Z

        )

-        response = yield self._send_request(
-            request,


nit: it's probably best to leave it as one arg on each line - it gets a bit impenetrable otherwise.

Addressed changes

richvdh

lgtm!

Redo of #4840

turt2live · 2019-04-19T18:46:15Z

ftr I don't think the backoff is working correctly here, but not enough to warrant a new issue. I'm seeing a stead 2.5Hz of requests without a trailing slash, and because I haven't updated yet it means that's 2.5Hz of failing requests.

richvdh · 2019-04-23T10:24:42Z

we didn't implement a backoff...

Remove trailing slashes from outbound federation requests

525dd02

anoadragon453 mentioned this pull request Mar 8, 2019

Make federation endpoints more tolerant of trailing slashes for some endpoints #4793

Merged

Retry certain federation requests on 404

64ff110

anoadragon453 added 2 commits March 8, 2019 18:23

Add changelog

f8740d5

lint

a5dd335

anoadragon453 requested a review from a team March 8, 2019 18:26

anoadragon453 marked this pull request as ready for review March 8, 2019 18:26

anoadragon453 removed the request for review from a team March 8, 2019 18:41

anoadragon453 added 7 commits March 11, 2019 17:07

Merge branch 'develop' into anoa/trailing_slashes_client

a8a028d

Merge branch 'develop' into anoa/trailing_slashes_client

f18dca2

We're calling different functions now

66f205e

Fix syntax error

802cb5d

and again

4868b12

Cleaner way of implementing trailing slashes

0ea8582

Correct argument name

97653ef

anoadragon453 requested a review from a team March 12, 2019 14:52

richvdh previously requested changes Mar 13, 2019

View reviewed changes

richvdh reviewed Mar 13, 2019

View reviewed changes

anoadragon453 added 3 commits March 13, 2019 11:14

Add workaround note

cf301e3

Fix paranthesis indent

7e75d96

Retry on 400:M_UNRECOGNIZED

7d053cf

anoadragon453 requested a review from richvdh March 13, 2019 12:38

richvdh previously requested changes Mar 13, 2019

View reviewed changes

anoadragon453 added 4 commits March 13, 2019 18:26

Switch to wrapper function around _send_request

09626bf

Fix syntax issues

5526b05

Add missing docstring detail

660b77f

Remove testing code

220607a

anoadragon453 added 10 commits March 13, 2019 20:17

i should have given up x2

45524f2

i should have given up x3

86c60bd

is this what purgatory feels like

9a2e22f

receiving a 400 caused an exception. handle it

b2df0e8

Correct var name

ecea5af

Better exception handling

621e7f3

lint

a8ad39e

Federation test fixed!

94cb793

Just return if not doing any trailing slash shennanigans

551ea11

Fix comments. v0.99.2 -> v0.99.3

c69df5d

anoadragon453 requested a review from richvdh March 20, 2019 11:32

richvdh previously requested changes Mar 20, 2019

View reviewed changes

anoadragon453 added 3 commits March 20, 2019 14:00

New test, fix issues

cd36a12

lint

bb52a2e

kwargs doesn't like commas on calling funcs either. TIL

2150151

anoadragon453 requested a review from richvdh March 20, 2019 14:33

richvdh previously requested changes Mar 21, 2019

View reviewed changes

Clean up backoff_on_404 and metehod calls

b41c2ea

anoadragon453 requested a review from richvdh March 21, 2019 14:50

richvdh approved these changes Mar 21, 2019

View reviewed changes

anoadragon453 merged commit 7bef97d into develop Mar 21, 2019

anoadragon453 deleted the anoa/trailing_slashes_client branch March 21, 2019 15:07

anoadragon453 added a commit that referenced this pull request Mar 26, 2019

Make federation endpoints more tolerant of trailing slashes v2 (#4935)

4a125be

Redo of #4840

anoadragon453 mentioned this pull request Sep 19, 2019

Handle trailing slashes at the end of paths matrix-org/dendrite#613

Closed

matrixbot mentioned this pull request Oct 30, 2024

Handle trailing slashes at the end of paths element-hq/dendrite#613

Closed

	try_trailing_slash_on_404=False):
	try_trailing_slash_on_404=False,
	):


		backoff_on_404 (bool): Back off if we get a 404

		try_trailing_slash_on_404 (bool): True if on a 404 response we

Uh oh!

Conversation

anoadragon453 commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoadragon453 commented Mar 13, 2019

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

turt2live commented Apr 19, 2019

Uh oh!

richvdh commented Apr 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anoadragon453 commented Mar 8, 2019 •

edited

Loading

codecov Bot commented Mar 8, 2019 •

edited

Loading