[pve-devel] applied: [RFC PATCH common] RESTEnvironment: better SIGCHLD handling in AnyEvent event loop

[Thomas Lamprecht]

On 20/02/2023 11:08, Dominik Csapak wrote:
> when we're in an API server that uses AnyEvent, we must postpone
> the worker_reaper, since it calls 'active_workers' which might already
> be called and then we're inside the lock twice (flocks are per process
> for us, see PVE::Tools::lock_file)
> 
> This resulted in an error like this:
> close (rename) atomic file '/var/log/pve/tasks/active' failed: No such file or directory
> 
> We use the fact that only 'pub' and 'priv' RESTEnvironment types are an
> api server with anyevent. For other types we call it like before.
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> Not super happy about the coupling between the RESTEnvironment and AnyEvent.
> We could try to just save the worker_reaper in 'self' and let the users
> of the env decide when to call it, but that would be more involved.
> 
> OTOH, we already do some anyevent specific things in PVE::Daemon
> (without depending on the AnyEvent package though)...
> 
> Also i did not find a way to dynamically find out if we're in an
> AnyEvent loop...
> 
>  debian/control             |  1 +
>  src/PVE/RESTEnvironment.pm | 13 ++++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/debian/control b/debian/control
> index 232a0e4..1c75985 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -3,6 +3,7 @@ Section: perl
>  Priority: optional
>  Maintainer: Proxmox Support Team <support at proxmox.com>
>  Build-Depends: debhelper (>= 12~),
> +               libanyevent-perl,

if we manage to guard calling into anyevent correctly it would not really be a
hard-dependency; OTOH, in practice not much (nothing?) would change so not really
that hard feelings either - especially as you mention the use in PVE::Daemon ..

>                 libclone-perl,
>                 libdevel-cycle-perl,
>                 libfilesys-df-perl,
> diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
> index bf89c12..c258b1e 100644
> --- a/src/PVE/RESTEnvironment.pm
> +++ b/src/PVE/RESTEnvironment.pm
> @@ -14,6 +14,7 @@ use IO::File;
>  use IO::Handle;
>  use IO::Select;
>  use POSIX qw(:sys_wait_h EINTR);
> +use AnyEvent;
>  
>  use PVE::Exception qw(raise raise_perm_exc);
>  use PVE::INotify;
> @@ -111,7 +112,17 @@ sub init {
>      die "unknown environment type"
>  	if !$type || $type !~ m/^(cli|pub|priv|ha)$/;
>  
> -    $SIG{CHLD} = $worker_reaper;
> +    my $has_anyevent = $type eq 'pub' || $type eq 'priv';
> +

meh, I'd like some more direct check, FWIW, we could check for `$INC{'AnyEvent.pm'}` if
that's any use here? I hoped $AnyEvent::MODEL could be used, but from a very quick test
it was uninitialized, maybe I held it wrong..

> +    $SIG{CHLD} = sub {
> +	# when we're in an api server, we have to postpone the call to worker_reaper, otherwise it
> +	# might interfere with running api calls
> +	if ($has_anyevent) {
> +	    AnyEvent::postpone { $worker_reaper->() };
> +	} else {
> +	    $worker_reaper->();
> +	}
> +    };
>  
>      # environment types
>      # cli  ... command started fron command line

Anyhow: applied for now, thanks! Finding a better way to detect if this is required would
be still nice though. if anybody got ideas please lets hear them ;-)