From 70b33965065f0e93eaba68e371896149c9405f51 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Wed, 19 Oct 2022 15:39:43 -0500
Subject: [PATCH] Explain `SynapseError` and `FederationError` better (#14191)

Explain `SynapseError` and `FederationError` better

Spawning from https://github.com/matrix-org/synapse/pull/13816#discussion_r993262622
---
 changelog.d/14191.doc                   |  1 +
 synapse/api/errors.py                   | 24 +++++++++++++++++++++---
 synapse/federation/federation_server.py |  8 ++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)
 create mode 100644 changelog.d/14191.doc

diff --git a/changelog.d/14191.doc b/changelog.d/14191.doc
new file mode 100644
index 000000000..6b0eeb1ae
--- /dev/null
+++ b/changelog.d/14191.doc
@@ -0,0 +1 @@
+Update docstrings of `SynapseError` and `FederationError` to bettter describe what they are used for and the effects of using them are.
diff --git a/synapse/api/errors.py b/synapse/api/errors.py
index e0873b191..400dd12ab 100644
--- a/synapse/api/errors.py
+++ b/synapse/api/errors.py
@@ -155,7 +155,13 @@ class RedirectException(CodeMessageException):
 
 class SynapseError(CodeMessageException):
     """A base exception type for matrix errors which have an errcode and error
-    message (as well as an HTTP status code).
+    message (as well as an HTTP status code). These often bubble all the way up to the
+    client API response so the error code and status often reach the client directly as
+    defined here. If the error doesn't make sense to present to a client, then it
+    probably shouldn't be a `SynapseError`. For example, if we contact another
+    homeserver over federation, we shouldn't automatically ferry response errors back to
+    the client on our end (a 500 from a remote server does not make sense to a client
+    when our server did not experience a 500).
 
     Attributes:
         errcode: Matrix error code e.g 'M_FORBIDDEN'
@@ -600,8 +606,20 @@ def cs_error(msg: str, code: str = Codes.UNKNOWN, **kwargs: Any) -> "JsonDict":
 
 
 class FederationError(RuntimeError):
-    """This class is used to inform remote homeservers about erroneous
-    PDUs they sent us.
+    """
+    Raised when we process an erroneous PDU.
+
+    There are two kinds of scenarios where this exception can be raised:
+
+    1. We may pull an invalid PDU from a remote homeserver (e.g. during backfill). We
+       raise this exception to signal an error to the rest of the application.
+    2. We may be pushed an invalid PDU as part of a `/send` transaction from a remote
+       homeserver. We raise so that we can respond to the transaction and include the
+       error string in the "PDU Processing Result". The message which will likely be
+       ignored by the remote homeserver and is not machine parse-able since it's just a
+       string.
+
+    TODO: In the future, we should split these usage scenarios into their own error types.
 
     FATAL: The remote server could not interpret the source event.
         (e.g., it was missing a required field)
diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py
index 28097664b..59e351595 100644
--- a/synapse/federation/federation_server.py
+++ b/synapse/federation/federation_server.py
@@ -481,6 +481,14 @@ class FederationServer(FederationBase):
                     pdu_results[pdu.event_id] = await process_pdu(pdu)
 
         async def process_pdu(pdu: EventBase) -> JsonDict:
+            """
+            Processes a pushed PDU sent to us via a `/send` transaction
+
+            Returns:
+                JsonDict representing a "PDU Processing Result" that will be bundled up
+                with the other processed PDU's in the `/send` transaction and sent back
+                to remote homeserver.
+            """
             event_id = pdu.event_id
             with nested_logging_context(event_id):
                 try: