Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix exception reporting due to HTTP request errors. #7556

Merged
merged 2 commits into from
May 22, 2020
Merged

Fix exception reporting due to HTTP request errors. #7556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12d4a80
Fix exception reporting due to HTTP request errors.
erikjohnston May 22, 2020
37b8a0a
Newsfile
erikjohnston May 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/7556.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Stop logging some expected HTTP request errors as exceptions.
13 changes: 9 additions & 4 deletions synapse/app/generic_worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

import synapse
import synapse.events
from synapse.api.errors import SynapseError
from synapse.api.errors import HttpResponseException, RequestSendFailed, SynapseError
from synapse.api.urls import (
CLIENT_API_PREFIX,
FEDERATION_PREFIX,
Expand Down Expand Up @@ -202,9 +202,14 @@ async def on_POST(self, request, device_id):
# is there.
auth_headers = request.requestHeaders.getRawHeaders(b"Authorization", [])
headers = {"Authorization": auth_headers}
result = await self.http_client.post_json_get_json(
self.main_uri + request.uri.decode("ascii"), body, headers=headers
)
try:
result = await self.http_client.post_json_get_json(
self.main_uri + request.uri.decode("ascii"), body, headers=headers
)
except HttpResponseException as e:
raise e.to_synapse() from e
except RequestSendFailed as e:
raise SynapseError(502, "Failed to talk to master") from e
Comment on lines +210 to +212
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"from e" is implicit when inside an except block... maybe it's worth including for clarity?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that is true: https://www.python.org/dev/peps/pep-3134/#explicit-exception-chaining

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is: https://www.python.org/dev/peps/pep-3134/#implicit-exception-chaining

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm ISWYM, they seem to do slightly different things. How does the result differ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, that is slightly more interesting question. TBH I was mostly going by the fact that we bother using six.raise_from elsewhere when wrapping like this.

Copy link
Member Author

@erikjohnston erikjohnston May 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it gets rid of some of the return deferred exceptions as we explicitly set the __cause__ and so ignore the implicit __context__?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asi in, I think the intention is that the implicit style is for "argh we failed to process this exception and raised another one" while the explicit style is for wrapping exceptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be right. In which case, there are probably lots of other places we should be doing the same.


return 200, result
else:
Expand Down
7 changes: 7 additions & 0 deletions synapse/handlers/federation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
Codes,
FederationDeniedError,
FederationError,
HttpResponseException,
RequestSendFailed,
SynapseError,
)
Expand Down Expand Up @@ -1036,6 +1037,12 @@ async def try_backfill(domains):
# TODO: We can probably do something more intelligent here.
return True
except SynapseError as e:
logger.info("Failed to backfill from %s because %s", dom, e)
continue
except HttpResponseException as e:
if 400 <= e.code < 500:
raise e.to_synapse_error()

logger.info("Failed to backfill from %s because %s", dom, e)
continue
except CodeMessageException as e:
Expand Down
7 changes: 7 additions & 0 deletions synapse/http/matrixfederationclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ def _handle_json_response(reactor, timeout_sec, request, response):
d = timeout_deferred(d, timeout=timeout_sec, reactor=reactor)

body = yield make_deferred_yieldable(d)
except TimeoutError as e:
logger.warning(
"{%s} [%s] Timed out reading response", request.txn_id, request.destination,
)
raise RequestSendFailed(e, can_retry=True) from e
except Exception as e:
logger.warning(
"{%s} [%s] Error reading response: %s",
Expand Down Expand Up @@ -424,6 +429,8 @@ def _send_request(
)

response = yield request_deferred
except TimeoutError as e:
raise RequestSendFailed(e, can_retry=True) from e
except DNSLookupError as e:
raise_from(RequestSendFailed(e, can_retry=retry_on_dns_fail), e)
except Exception as e:
Expand Down