Wait for synapse to start by using sd_notify.#943
Conversation
Currently SyTest waits for synapse process to fully start by repeatedly checking if it can connect to the HTTP socket. This won't work for starting worker processest that do not listen on any HTTP socket. Instead, we use the `sd_notify` mechanism. This works by SyTest binding to a unix socket, passing the socket address to Synapse via env, and waiting to receive a `READY=1` notification on the socket.
|
Oh, this does't work with pydron of course. Whoops |
|
Ok, so I've made nothing use it yet, and instead will use it when I rejig things to remove |
| )->else_with_f( sub { | ||
| my ( $f ) = @_; | ||
|
|
||
| # We need to manually kill child procs here as we don't seem to have | ||
| # registered the on finish handler yet. | ||
| $self->kill_and_await_finish()->then( sub { | ||
| $f | ||
| }) | ||
| } ); |
There was a problem hiding this comment.
this feels wrong, for a couple of reasons:
- failure to start one subprocess should not necessarily mean that all other subprocesses get killed (at least at the abstract level of a ProcessManager class)
- it's asymmetric with
_start_process_and_await_connectable.
what is the "finish handler" of which the comment speaks?
There was a problem hiding this comment.
Ah, turns out that there is a timeout several layers up. Its set to 60s so I probably got bored waiting before it fired :/
| # Create a random abstract socket name. Abstract sockets start with a null | ||
| # byte. | ||
| my $random_id = join "", map { chr 65 + rand 25 } 1 .. 20; | ||
| my $path = "\0sytest-$random_id.sock"; | ||
|
|
||
| # We replace null byte with '@' to allow us to pass it in via env. (This is | ||
| # as per the sd_notify spec). | ||
| $env->{"NOTIFY_SOCKET"} = $path =~ s/\0/\@/rg; |
There was a problem hiding this comment.
cleaner to do this outside _await_ready_notification (possibly in another helper function) and just pass in the $path to _await_ready_notification ?
There was a problem hiding this comment.
I had a look at this, and basically either a) you move it to a function that accepts $env which I don't think buys you very much, or b) move this block of code up to the _start_process_and_await.. function which just makes it a bit messy.
I kind of like have all the sd notify bits in one place tbh. Maybe it'd be better to rename it _create_socket_and_await_notification or something?
| # Create a random abstract socket name. Abstract sockets start with a null | ||
| # byte. | ||
| my $random_id = join "", map { chr 65 + rand 25 } 1 .. 20; | ||
| my $path = "\0sytest-$random_id.sock"; | ||
|
|
||
| # We replace null byte with '@' to allow us to pass it in via env. (This is | ||
| # as per the sd_notify spec). | ||
| $env->{"NOTIFY_SOCKET"} = $path =~ s/\0/\@/rg; |
There was a problem hiding this comment.
this looks like it modifies $path itself, which isn't what you want?
There was a problem hiding this comment.
Aahaha. Hahaha. Ha.
😭
The r flag there means don't overwrite the variable. Apparently that is the way that you do non-destructive replace or something. Would my $path_env = $path; $path_env =~ s/\0/\@/g; work, do you know?
There was a problem hiding this comment.
oh right. Yes, that should work. As, I think, should ($env->{"NOTIFY_SOCKET"} = $path) =~ s/\0/\@/g;
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Turns out there is already a time out.
erikjohnston
force-pushed
the
erikj/sd_notify_start
branch
from
f965623 to
393c530
Compare
Currently SyTest waits for synapse process to fully start by repeatedly checking if it can connect to the HTTP socket. This won't work for starting worker processest that do not listen on any HTTP socket. Instead, we use the `sd_notify` mechanism. This works by SyTest binding to a unix socket, passing the socket address to Synapse via env, and waiting to receive a `READY=1` notification on the socket.
…c_release_1_21_x * origin/release-v1.20.1: (27 commits) Pin Alien::Sodium to an old version, to fix install errors (#949) Disable rate limiting on Dendrite (#947) Wait for synapse to start by using sd_notify. (#943) Fix bad tests (#939) Add ability to exclude tests for deprecated endpoints (#881) Abide by the spec when making /keys/query requests When putting device keys, may the JSON body spec compliant Remove trailing slashes from /state_ids and /backfill in ACL tests (#936) Fix /send server ACL test to match spec (#935) Call matrix_get_e2e_keys correctly Removed unused params from pydron invocation (#931) Deflake 'Server correctly resyncs when server leaves and rejoins a room' (#932) Dendrite configuration format v1 (#921) Disable TLS validation for Dendrite Set Dendrite loglevel to trace (#933) Merge Synapse::ViaHaproxy and -I Synapse::Dendron (#930) Use ProcessManager in Synapse.pm (#929) Add redis support to synapse docker image (#928) Update the README for the docker images (#927) Some minor cleanups to the startup scripts (#926) ...
2 participants
Currently SyTest waits for synapse process to fully start by repeatedly
checking if it can connect to the HTTP socket. This won't work for
starting worker processest that do not listen on any HTTP socket.
Instead, we use the
sd_notifymechanism. This works by SyTest bindingto a unix socket, passing the socket address to Synapse via env, and
waiting to receive a
READY=1notification on the socket.