New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up the way logcontexts and threads work in the pushers #4075

Merged
merged 10 commits into from Oct 24, 2018

Conversation

2 participants
@richvdh
Member

richvdh commented Oct 20, 2018

This was all a bit of an inconsistent confusing mess, and led to things being
done with no logcontext and leaky logcontexts.

The starting point is to note that each pusher has its own loop which runs for
as long as it has work to do. This should run in its own background thread with
its own logcontext, so we start by making that so (see _start_processing and
_process in each of HttpPusher and EmailPusher).

Once we've done that, it turns out that the hooks to notify the pushers
(on_started, on_new_notifications, on_new_receipts) may as well be
synchronous because all they have to do is set off the background process (a
slight exception is HttpPusher.on_new_receipts, which needs its own
background process to keep it symetric with the others). That
means that we can remove the run_as_background_process calls from PusherPool.

Sorry about the size of this PR - it's broken down into commits doing roughly the above, preceeded by a bunch of refactoring to make it tractable.

Fixes: #4066

@richvdh richvdh requested review from matrix-org/synapse-core and removed request for matrix-org/synapse-core Oct 20, 2018

richvdh added some commits Oct 22, 2018

Run PusherPool.start as a background process
We don't do anything with the result, so this is needed to give this code a
logcontext.
Factor PusherPool._start_pusher out of _start_pushers
... and use it from start_pusher_by_id. This mostly simplifies
start_pusher_by_id.
Rename _refresh_pusher
This is public (or at least, called from outside the class), so ought to have a
better name.
move get_all_pushers call down
simplifies the interface to _start_pushers
Give pushers their own background logcontext
Each pusher has its own loop which runs for as long as it has work to do. This
should run in its own background thread with its own logcontext, as other
similar loops elsewhere in the system do - which means that CPU usage is
consistently attributed to that loop, rather than to whatever request happened
to start the loop.
Remove redundant run_as_background_process() from pusherpool
`on_new_notifications` and `on_new_receipts` in `HttpPusher` and `EmailPusher`
now always return synchronously, so we can remove the `defer.gatherResults` on
their results, and the `run_as_background_process` wrappers can be removed too
because the PusherPool methods will now complete quickly enough.
Make on_started synchronous too
This brings it into line with on_new_notifications and on_new_receipts. It
requires a little bit of hoop-jumping in EmailPusher to load the throttle
params before the first loop.
@richvdh

This comment has been minimized.

Member

richvdh commented Oct 23, 2018

worth noting that github is showing these commits in the wrong order.

@richvdh richvdh requested a review from matrix-org/synapse-core Oct 23, 2018

@erikjohnston

This looks like a big improvement. I'm not hugely thrilled about the disconnect between checks to is_processing and actually setting is_processing, it looks like it could be quite a large footgun if we're not careful. Could we add a check within the processing loops too? So that yes we normally bail out early, but for paranoia's sake we double check before actually setting it and doing the work?

@@ -192,6 +192,9 @@ def _on_new_receipts(self, min_stream_id, max_stream_id, affected_room_ids):
@defer.inlineCallbacks
def start_pusher_by_id(self, app_id, pushkey, user_id):
"""Look up the details for the given pusher, and start it"""
if not self._start_pushers:

This comment has been minimized.

@erikjohnston

erikjohnston Oct 23, 2018

Member

Isn't this a function?

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

fucksticks. who would have a a field and a function with the same name modulo an underscore

richvdh added some commits Oct 24, 2018

sanity-check the is_processing flag
... and rename it, for even more sanity

@richvdh richvdh requested a review from erikjohnston Oct 24, 2018

@richvdh

This comment has been minimized.

Member

richvdh commented Oct 24, 2018

build has failed due to pep8 due to new flake8 version - will investigate separately.

@richvdh richvdh merged commit e0b9d5f into develop Oct 24, 2018

4 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details

@richvdh richvdh added this to To Do in Backend Core Team via automation Oct 25, 2018

@richvdh richvdh moved this from To Do to Done - Operations in Backend Core Team Oct 25, 2018

@richvdh richvdh deleted the rav/fix_pusher_logcontexts branch Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment