This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Clean up the way logcontexts and threads work in the pushers #4075
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richvdh
force-pushed
the
rav/fix_pusher_logcontexts
branch
from
212d615 to
1ff43b6
Compare
This is public (or at least, called from outside the class), so ought to have a better name.
... and use it from start_pusher_by_id. This mostly simplifies start_pusher_by_id.
simplifies the interface to _start_pushers
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.
`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.
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.
We don't do anything with the result, so this is needed to give this code a logcontext.
richvdh
force-pushed
the
rav/fix_pusher_logcontexts
branch
from
1ff43b6 to
abd9914
Compare
|
worth noting that github is showing these commits in the wrong order. |
erikjohnston
suggested changes
Oct 23, 2018
There was a problem hiding this comment.
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?
synapse/push/pusherpool.py
Outdated
| @@ -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: | |||
There was a problem hiding this comment.
fucksticks. who would have a a field and a function with the same name modulo an underscore
... and rename it, for even more sanity
erikjohnston
approved these changes
Oct 24, 2018
|
build has failed due to pep8 due to new flake8 version - will investigate separately. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_processingand_processin each ofHttpPusherandEmailPusher).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 besynchronous because all they have to do is set off the background process (a
slight exception is
HttpPusher.on_new_receipts, which needs its ownbackground process to keep it symetric with the others). That
means that we can remove the
run_as_background_processcalls 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