Add admin handler to list of handlers used for background tasks#17847
Add admin handler to list of handlers used for background tasks#17847MadLittleMods merged 6 commits intoelement-hq:developfrom
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.
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.
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.
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 <madlittlemods@gmail.com>
|
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:
TaskSchedulerrun_as_background_process(...)wait_for_background_processes(...)reactor