-
Notifications
You must be signed in to change notification settings - Fork 223
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
Add admin handler to list of handlers used for background tasks #17847
Conversation
tests/rest/admin/test_user.py
Outdated
self.assertEqual(channel.code, 200) | ||
id = channel.json_body.get("redact_id") | ||
|
||
for _ in range(100): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very janky way of saying "please main process wait until the worker process has finished executing the redaction task before running the rest of the test code." I would love to know the actual way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sort of thing might be good enough. For example, look at how wait_for_background_updates()
works.
Perhaps just change this to a while
loop with a timeout (example in wait_on_thread()
).
Could be updated to use the TaskScheduler
functions directly instead of the HTTP requests but keeping it as a black box test is also great 👍
One alternative is that because this particular background task is producing a bunch of redactions which you end up checking anyway, is to just /sync
until you see a redaction for all the events. I don't know if we have something for that in Synapse but Complement has MustSyncUntil
for example that we could emulate over here.
It looks like the TaskScheduler
uses run_as_background_process(...)
behind the scenes. Here is a previous PR where I wanted to wait for things that ran in a background process:
- Process previously failed backfill events in the background matrix-org/synapse#15585 (comment)
- Process previously failed backfill events in the background matrix-org/synapse#15585 (comment)
Although, I'm unable to get the trick suggested there to work here. Here is what I tried:
worker = self.make_worker_hs(...)
[...]
# Wait for the next run of the scheduler loop
worker.get_reactor().advance((TaskScheduler.SCHEDULE_INTERVAL_MS / 1000))
# Ensure `run_as_background_process(...)` has a chance to run (essentially
# `wait_for_background_processes()`)
worker.get_reactor().pump((0.1,))
# Even adding this for good measure, doesn't work
self.reactor.pump((0.1,))
As an aside (for my own reference), reactor.pump([1])
vs reactor.advance(1)
seem to be the same thing. I wonder why both exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opted to use a while loop - hopefully this is sufficient? I went down the rabbit hole of trying to poke at the reactor before resorting to what I originally submitted, so if the while loop is acceptable let's leave it at that :)
Co-authored-by: Eric Eastwood <[email protected]>
Thanks for the review @MadLittleMods! I've made the suggested changes - if you're satisfied can you hit the merge button when you get a chance? I no longer have the power to do so :) |
Fixes #17823
While we're at it, makes a change where the redactions are sent as the admin if the user is not a member of the server (otherwise these fail with a "User must be our own" message).
Dev notes
Keywords:
TaskScheduler
run_as_background_process(...)
wait_for_background_processes(...)
reactor