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

Refactor request handling wrappers #3203

Merged
merged 9 commits into from
May 10, 2018
Merged

Refactor request handling wrappers #3203

merged 9 commits into from
May 10, 2018

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented May 10, 2018

The end goal here is to separate the logging bits and the exception handling
bits of the request_handler() decorator because (a) simplicity and (b) I'd like
to use the logging bits in places where JSON error handling is inappropriate.

richvdh added 9 commits May 9, 2018 19:55
@richvdh
Move RequestsMetrics to its own file
18e144f 
This is useful in its own right, because server.py is full of stuff; but more
importantly, I want to do some refactoring that will cause a circular reference
as it is.
@richvdh
Move request_id management into SynapseRequest
8460e48
@richvdh
Make RequestMetrics take a raw time rather than a clock
09b29f9 
... which is going to make it easier to move around.
@richvdh
Move RequestMetrics handling into SynapseRequest.processing()
c6f7302 
It fits quite nicely here, and opens the path to getting rid of the
"include_metrics" mess.
@richvdh
Bump requests_counter in wrapped_request_handler
b8700dd 
less magic
@richvdh
Move outgoing_responses_counter handling to RequestMetrics
49e5a61 
it's much neater there.
@richvdh
Remove include_metrics param
9589a19 
The metrics are now available via the request, so this is redundant and can go
away at last.
@richvdh
Factor wrap_request_handler_with_logging out of wrap_request_handler
09f570b 
... so that it can be used on non-JSON endpoints
@richvdh
Remove redundant request_handler decorator
645cb4b 
This is needless complexity; we might as well use the wrapper directly.

Also rename wrap_request_handler->wrap_json_request_handler.
@richvdh richvdh force-pushed the rav/refactor_request_handler branch from 9d01771 to 645cb4b Compare May 10, 2018 11:21
@erikjohnston
Copy link
Member

I'm a bit confused at the splitting out of wrap_json_request_handler and wrap_request_handler_with_logging. You say its so that you can use wrap_request_handler_with_logging for non-JSON endpoints, but you don't actually use it like that.

Similarly, I'm also wary of the rename of wrap_json_request_handler since its being used in places that don't operate on JSON

@erikjohnston
Copy link
Member

(Functionally it looks OK though)

@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston May 10, 2018
@richvdh
Copy link
Member Author

richvdh commented May 10, 2018

You say its so that you can use wrap_request_handler_with_logging for non-JSON endpoints, but you don't actually use it like that.

Well, yes; it's still on its way. Basically I now also have a wrap_html_request_handler which formats exceptions as HTML for browsers rather than JSON for matrix clients.

Similarly, I'm also wary of the rename of wrap_json_request_handler since its being used in places that don't operate on JSON.

The point is less about what they operate on, and more about what they are expected to return, particularly in error conditions. Suggestions for better names welcome. (And yes, some of the media endpoints do look like maybe they should be doing something different on error conditions, but I'm not going to go through and figure them out)

@erikjohnston
Copy link
Member

Ok, well I guess lets see how it turns out once everything has landed.

@richvdh richvdh merged commit 7b41100 into develop May 10, 2018
neilisfragile added a commit that referenced this pull request May 18, 2018
@neilisfragile
Merge tag 'v0.29.1'
ab5e888 
Changes in synapse v0.29.1 (2018-05-17)
==========================================
Changes:

* Update docker documentation (PR #3222)

Changes in synapse v0.29.0 (2018-05-16)
===========================================
Not changes since v0.29.0-rc1

Changes in synapse v0.29.0-rc1 (2018-05-14)
===========================================

Notable changes, a docker file for running Synapse (Thanks to @kaiyou!) and a
closed spec bug in the Client Server API. Additionally further prep for Python 3
migration.

Potentially breaking change:

* Make Client-Server API return 401 for invalid token (PR #3161).

  This changes the Client-server spec to return a 401 error code instead of 403
  when the access token is unrecognised. This is the behaviour required by the
  specification, but some clients may be relying on the old, incorrect
  behaviour.

  Thanks to @NotAFile for fixing this.

Features:

* Add a Dockerfile for synapse (PR #2846) Thanks to @kaiyou!

Changes - General:

* nuke-room-from-db.sh: added postgresql option and help (PR #2337) Thanks to @rubo77!
* Part user from rooms on account deactivate (PR #3201)
* Make 'unexpected logging context' into warnings (PR #3007)
* Set Server header in SynapseRequest (PR #3208)
* remove duplicates from groups tables (PR #3129)
* Improve exception handling for background processes (PR #3138)
* Add missing consumeErrors to improve exception handling (PR #3139)
* reraise exceptions more carefully (PR #3142)
* Remove redundant call to preserve_fn (PR #3143)
* Trap exceptions thrown within run_in_background (PR #3144)

Changes - Refactors:

* Refactor /context to reuse pagination storage functions (PR #3193)
* Refactor recent events func to use pagination func (PR #3195)
* Refactor pagination DB API to return concrete type (PR #3196)
* Refactor get_recent_events_for_room return type (PR #3198)
* Refactor sync APIs to reuse pagination API (PR #3199)
* Remove unused code path from member change DB func (PR #3200)
* Refactor request handling wrappers (PR #3203)
* transaction_id, destination defined twice (PR #3209) Thanks to @damir-manapov!
* Refactor event storage to prepare for changes in state calculations (PR #3141)
* Set Server header in SynapseRequest (PR #3208)
* Use deferred.addTimeout instead of time_bound_deferred (PR #3127, #3178)
* Use run_in_background in preference to preserve_fn (PR #3140)

Changes - Python 3 migration:

* Construct HMAC as bytes on py3 (PR #3156) Thanks to @NotAFile!
* run config tests on py3 (PR #3159) Thanks to @NotAFile!
* Open certificate files as bytes (PR #3084) Thanks to @NotAFile!
* Open config file in non-bytes mode (PR #3085) Thanks to @NotAFile!
* Make event properties raise AttributeError instead (PR #3102) Thanks to @NotAFile!
* Use six.moves.urlparse (PR #3108) Thanks to @NotAFile!
* Add py3 tests to tox with folders that work (PR #3145) Thanks to @NotAFile!
* Don't yield in list comprehensions (PR #3150) Thanks to @NotAFile!
* Move more xrange to six (PR #3151) Thanks to @NotAFile!
* make imports local (PR #3152) Thanks to @NotAFile!
* move httplib import to six (PR #3153) Thanks to @NotAFile!
* Replace stringIO imports with six (PR #3154, #3168) Thanks to @NotAFile!
* more bytes strings (PR #3155) Thanks to @NotAFile!

Bug Fixes:

* synapse fails to start under Twisted >= 18.4 (PR #3157)
* Fix a class of logcontext leaks (PR #3170)
* Fix a couple of logcontext leaks in unit tests (PR #3172)
* Fix logcontext leak in media repo (PR #3174)
* Escape label values in prometheus metrics (PR #3175, #3186)
* Fix 'Unhandled Error' logs with Twisted 18.4 (PR #3182) Thanks to @Half-Shot!
* Fix logcontext leaks in rate limiter (PR #3183)
* notifications: Convert next_token to string according to the spec (PR #3190) Thanks to @mujx!
* nuke-room-from-db.sh: fix deletion from search table (PR #3194) Thanks to @rubo77!
* add guard for None on purge_history api (PR #3160) Thanks to @krombel!
@richvdh richvdh deleted the rav/refactor_request_handler branch July 10, 2018 12:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erikjohnston erikjohnston erikjohnston approved these changes

Assignees

@richvdh richvdh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@richvdh @erikjohnston