Better handle 404 response for federation /send/ #1866

Merged
merged 4 commits into from Feb 1, 2017

Projects

None yet

2 participants

@erikjohnston
Member

No description provided.

@erikjohnston erikjohnston Better handle 404 response for federation /send/
ae7a132
synapse/util/retryutils.py
def __enter__(self):
pass
def __exit__(self, exc_type, exc_val, exc_tb):
valid_err_code = False
if exc_type is not None and issubclass(exc_type, CodeMessageException):
- valid_err_code = exc_val.code != 429 and 0 <= exc_val.code < 500
+ if exc_val.code < 400:
@NegativeMjark
NegativeMjark Jan 31, 2017 edited Contributor

Maybe chuck in a short comment explaining what's going on here? Just to explain what a valid_err_code is and why 404, 429 and >500 are special?

erikjohnston added some commits Jan 31, 2017
@erikjohnston erikjohnston Comment
85c5901
@erikjohnston erikjohnston Comment
4c0ec15
synapse/util/retryutils.py
+ # APIs may expect to never received e.g. a 404. It's important to
+ # handle 404 as some remote servers will return a 404 when the HS
+ # has been decommissioned.
+ if exc_val.code < 400:
@NegativeMjark
NegativeMjark Jan 31, 2017 Contributor

Isn't the exc_val.code < 400 redundant because of the exc_val.code < 500 ?

@erikjohnston
erikjohnston Jan 31, 2017 Member

Yeah, though personally I quite like the fact that each case of code is handled in order. I can change it if you think it makes it clearer?

@NegativeMjark
NegativeMjark Jan 31, 2017 Contributor

I find it misleading because it makes it seem as though 4xx are handled differently to 2xx/3xx.

@NegativeMjark
Contributor

LGTM apart from the possibly redundant check?

@erikjohnston erikjohnston Remove explicit < 400 check as apparently this is confusing
fe08db2
@NegativeMjark

LGTM

@erikjohnston
Member

@matrixbot retest this please

@erikjohnston erikjohnston merged commit 06567ec into develop Feb 1, 2017

7 of 8 checks passed

Sytest Dendron (Commit) Build #1451 origin/erikj/device_list_fixes failed in 2 min 7 sec
Details
Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #2268 origin/erikj/device_list_fixes succeeded in 8 min 19 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #2337 origin/erikj/device_list_fixes succeeded in 4 min 22 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment