diff --git a/NEWS.adoc b/NEWS.adoc index 51c221fc52..ac53fbe8fc 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -48,6 +48,9 @@ https://github.com/networkupstools/nut/milestone/13 * `libnutconf`: character reads from a TCP socket now use the Windows socket API, matching string reads. Previously they failed with a bad file descriptor error on Windows. [issue #2293] + * `upssched` runs commands from expired timers asynchronously, so a slow + command no longer delays other timers or timer requests. The daemon + waits for these commands before exiting. [issue #3115, PR #3769] * Now `upssched -l` preserves timer expiry timestamps beyond January 2038 on systems with 64-bit `time_t` and 32-bit `long`. [issue #3096, PR #3669] * PyNUT: `GetUPSCommands()` now requests command descriptions with the diff --git a/UPGRADING.adoc b/UPGRADING.adoc index b68421c75b..b88dc6a112 100644 --- a/UPGRADING.adoc +++ b/UPGRADING.adoc @@ -31,6 +31,13 @@ command line, in order to quickly pick up any other removed option flags. Changes from 2.8.5 to 2.8.6 --------------------------- +- `upssched` timer commands now run asynchronously. Commands from different + timers, or successive invocations of the same timer name, may overlap. + Scripts which relied on serial execution must arrange their own locking. + On idle exit, the timer daemon waits for its directly launched commands. + Immediate `EXECUTE` and unmatched `CANCEL-TIMER` command handling retain + their synchronous behaviour. [issue #3115, PR #3769] + - PLANNED: Keep track of any further API clean-up? - The `PACKAGE_VERSION` and `PACKAGE_STRING` macros in the generated diff --git a/clients/upssched.c b/clients/upssched.c index 5365e3c52b..76de4526c3 100644 --- a/clients/upssched.c +++ b/clients/upssched.c @@ -45,7 +45,7 @@ * * timers can be cancelled at any time before they trigger * - * the daemon will shut down automatically when no more timers are active + * the daemon shuts down after all timers and their direct commands finish * */ @@ -149,17 +149,30 @@ static OVERLAPPED connect_overlapped; #define US_SOCK_BUF_LEN 256 #define US_MAX_READ 128 +typedef struct command_s { +#ifndef WIN32 + pid_t pid; +#else + intptr_t pid; +#endif + char *name; + struct command_s *next; +} command_t; + +static command_t *commands = NULL; + /* --- server functions --- */ -static void exec_cmd(const char *cmd) +static void exec_cmd(const char *cmd, int asynchronous) { #ifndef WIN32 int waitstatus = 0; pid_t pid, waitret; #else - int err = 0; + intptr_t err = 0; #endif char **argv = NULL; + command_t *child; if (cmdscript_argc == 0) { upslogx(LOG_ERR, "No CMDSCRIPT defined, cannot execute command: %s", NUT_STRARG(cmd)); @@ -177,6 +190,15 @@ static void exec_cmd(const char *cmd) } if (pid > 0) { + if (asynchronous) { + child = (command_t *)xcalloc(1, sizeof(*child)); + child->pid = pid; + child->name = xstrdup(cmd); + child->next = commands; + commands = child; + return; + } + /* parent process - wait for child */ waitret = waitpid(pid, &waitstatus, 0); if (waitret < 0) { @@ -221,10 +243,20 @@ static void exec_cmd(const char *cmd) exit(EXIT_FAILURE); #else /* WIN32 */ /* Use _spawnvp for Windows */ - err = _spawnvp(_P_WAIT, cmdscript_argv[0], (const char * const *)argv); + err = _spawnvp(asynchronous ? _P_NOWAIT : _P_WAIT, + cmdscript_argv[0], (const char * const *)argv); + if (asynchronous && err != -1) { + child = (command_t *)xcalloc(1, sizeof(*child)); + child->pid = err; + child->name = xstrdup(cmd); + child->next = commands; + commands = child; + free(argv); + return; + } if (err != -1) { upslogx(LOG_INFO, "Execute command %s \"%s\" OK", NUT_STRARG(cmdscript_concat), NUT_STRARG(cmd)); - upsdebugx(3, "%s: returned status %d", __func__, err); + upsdebugx(3, "%s: returned status %d", __func__, (int)err); } else { upslog_with_errno(LOG_ERR, "Execute command %s \"%s\" failure", NUT_STRARG(cmdscript_concat), NUT_STRARG(cmd)); } @@ -317,6 +349,57 @@ static char* collect_string(char **string_arr, char *logtag, char *sep, size_t * return buf; } +/* Poll only commands owned by timer dispatch; do not collect other children. */ +static void reap_commands(void) +{ + command_t **link = &commands, *child; + + while ((child = *link) != NULL) { +#ifndef WIN32 + int status = 0; + pid_t ret = waitpid(child->pid, &status, WNOHANG); + + if (ret == 0 || (ret < 0 && errno == EINTR)) { + link = &child->next; + continue; + } + if (ret < 0) { + upslog_with_errno(LOG_ERR, "waitpid(%d) failed", (int)child->pid); + } else if (WIFEXITED(status)) { + if (WEXITSTATUS(status)) { + upslogx(LOG_INFO, "exec_cmd(%s '%s') returned %d", + NUT_STRARG(cmdscript_concat), child->name, WEXITSTATUS(status)); + } + } else if (WIFSIGNALED(status)) { + upslogx(LOG_WARNING, "exec_cmd(%s '%s') terminated with signal %d", + NUT_STRARG(cmdscript_concat), child->name, WTERMSIG(status)); + } else { + upslogx(LOG_ERR, "Execute command failure: %s '%s'", + NUT_STRARG(cmdscript_concat), child->name); + } +#else + DWORD status = 0; + DWORD ret = WaitForSingleObject((HANDLE)child->pid, 0); + + if (ret == WAIT_TIMEOUT) { + link = &child->next; + continue; + } + if (ret != WAIT_OBJECT_0 || !GetExitCodeProcess((HANDLE)child->pid, &status)) { + upslogx(LOG_ERR, "Cannot collect command '%s': Windows error %lu", + child->name, (unsigned long)GetLastError()); + } else if (status) { + upslogx(LOG_INFO, "exec_cmd(%s '%s') returned %lu", + NUT_STRARG(cmdscript_concat), child->name, (unsigned long)status); + } + CloseHandle((HANDLE)child->pid); +#endif + *link = child->next; + free(child->name); + free(child); + } +} + static void exec_cmd_timer(ttype_t *item) { char *upsnames = NULL, *notifytypes = NULL, *notifymsgs = NULL; @@ -352,7 +435,7 @@ static void exec_cmd_timer(ttype_t *item) if (nut_debug_level) upslogx(LOG_INFO, "Executing command by timer: %s\t[%s]\t[%s]\t[%s]", item->name, NUT_STRARG(notifytypes), NUT_STRARG(upsnames), NUT_STRARG(notifymsgs)); - exec_cmd(item->name); + exec_cmd(item->name, 1); upsdebugx(3, "%s: returned from exec_cmd()", __func__); /* Timer process should not retain random envvars */ @@ -436,8 +519,13 @@ static void checktimers(void) upsdebugx(3, "%s: starting", __func__); - /* if the queue is empty we might be ready to exit */ - if (!thead) { + reap_commands(); + if (commands) { + emptyctr = 0; + } + + /* Running commands keep the daemon alive, even with no queued timers. */ + if (!thead && !commands) { emptyctr++; @@ -726,7 +814,7 @@ static void cancel_timer(const char *name, const char *cname, const char *notify if (nut_debug_level) upslogx(LOG_INFO, "Cancel %s, event: %s", name, cname); - exec_cmd(cname); + exec_cmd(cname, 0); } } @@ -2419,7 +2507,7 @@ static void parse_at(const char *ntype, const char *un, const char *cmd, if (nut_debug_level) upslogx(LOG_INFO, "Executing command: %s", ca1); - exec_cmd(ca1); + exec_cmd(ca1, 0); return; } @@ -2649,6 +2737,16 @@ static void clean_exit(void) ttype_t *tcurr, *tnext; conn_t *ccurr, *cnext; size_t i; + command_t *child; + + while ((child = commands) != NULL) { + commands = child->next; +#ifdef WIN32 + CloseHandle((HANDLE)child->pid); +#endif + free(child->name); + free(child); + } /* Flush *our* output before possibly failing in third-party code * (e.g. SSL libs), so client consumers have a chance to see it */ diff --git a/docs/man/upssched.conf.txt b/docs/man/upssched.conf.txt index be3012321d..99afa4c580 100644 --- a/docs/man/upssched.conf.txt +++ b/docs/man/upssched.conf.txt @@ -18,12 +18,13 @@ IMPORTANT NOTES * Contents of this file should be pure ASCII (character codes not in range would be ignored with a warning message). -* Command execution is synchronous (with the called tool process in case - of `EXECUTE` directive, or with the timer process). Consider using your - system shell abilities like `&` to send long-duration handling to the - background and let `upssched` timer daemon continue. This should not - impact `upsmon` daemon, which handles each notification in a separate - sub-process (and so not a problem for immediate `EXECUTE` events). +* Commands launched by expired timers run asynchronously and may overlap, + including invocations of the same timer name. Use locking in scripts + which require serial execution. On idle exit, the timer daemon waits until + its directly launched commands finish, then follows its idle grace period. + Immediate `EXECUTE` commands wait in the calling tool process. The optional + command for `CANCEL-TIMER` with no matching timer still waits in the timer + daemon. `upsmon` handles notifications in separate sub-processes. CONFIGURATION DIRECTIVES ------------------------ diff --git a/docs/man/upssched.txt b/docs/man/upssched.txt index 0dbd2651e4..b6960a903a 100644 --- a/docs/man/upssched.txt +++ b/docs/man/upssched.txt @@ -81,13 +81,15 @@ If you also want to continue writing to the syslog, just add it in: For a full list of notify flags, see the linkman:upsmon[8] documentation. -Please note that command execution is synchronous (with the called `upssched` -tool process in case of `EXECUTE` directive, or with the timer process). -Consider using your system shell abilities like `&` to send long-duration -handling to the background and let `upssched` timer daemon continue. -This should not impact `upsmon` daemon, which handles each notification -in a separate sub-process (and so not a problem for immediate `EXECUTE` -events). +Commands launched by expired timers run asynchronously. A long-running command +therefore does not delay other timers or requests to list, start or cancel +pending timers. Commands may overlap, including successive invocations of the +same timer name. Scripts which require serial execution must arrange their +own locking. + +Immediate `EXECUTE` commands wait in the calling tool process. The optional +command for a `CANCEL-TIMER` with no matching timer still waits in the timer +daemon. `upsmon` handles each notification in a separate sub-process. CONFIGURATION ------------- @@ -157,12 +159,13 @@ It will come and go from the process list as necessary. When a new timer is started, a process will be forked to actually watch the clock and eventually start the CMDSCRIPT. When a timer triggers, it is removed from the queue. Cancelling a timer will also remove it from the queue. When -no timers are present in the queue, the background process exits. +no timers are present in the queue and all commands launched by timers have +finished, the background process exits after its idle grace period. This means that you will only see upssched running when one of two things is happening: - - There's a timer of some sort currently running + - There's a pending timer or a command launched by a timer still running - upsmon just called it, and you managed to catch the brief instance The final optimization handles the possibility of trying to cancel a timer @@ -180,7 +183,7 @@ runs in foreground mode. NOTE: Unlike some other NUT daemons, `upssched` with enabled debug does not stop reporting on `stderr`! It forks a background process with the first call as an event handler, which exits soon after all -tracked timers have elapsed and were handled (if needed). +tracked timers have elapsed and their directly launched commands have finished. *UPSNAME* and *NOTIFYTYPE* are required, as detailed above. They are set by `upsmon` when it calls `upssched` as its choice of `NOTIFYCMD`. diff --git a/tests/Makefile.am b/tests/Makefile.am index 3577f83028..f862eea54e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -154,6 +154,12 @@ upssched_list_timers_test_CFLAGS = $(AM_CFLAGS) -I$(top_srcdir)/clients upssched_list_timers_test_LDADD = $(top_builddir)/common/libcommonversion.la $(NUT_LIBCOMMON) $(NETLIBS) upssched_list_timers_test_DEPENDENCIES = $(NUT_LIBCOMMON) $(top_builddir)/common/libcommonversion.la $(top_srcdir)/clients/upssched.c $(top_srcdir)/clients/upssched.h +TESTS += upssched-async-test +upssched_async_test_SOURCES = upssched-async-test.c +upssched_async_test_CFLAGS = $(AM_CFLAGS) -I$(top_srcdir)/clients +upssched_async_test_LDADD = $(top_builddir)/common/libcommonversion.la $(NUT_LIBCOMMON) $(NETLIBS) +upssched_async_test_DEPENDENCIES = $(NUT_LIBCOMMON) $(top_builddir)/common/libcommonversion.la $(top_srcdir)/clients/upssched.c $(top_srcdir)/clients/upssched.h + TESTS += statetest statetest_SOURCES = statetest.c statetest_LDADD = $(NUT_LIBCOMMON) diff --git a/tests/upssched-async-test.c b/tests/upssched-async-test.c new file mode 100644 index 0000000000..8310e12f52 --- /dev/null +++ b/tests/upssched-async-test.c @@ -0,0 +1,65 @@ +/* upssched-async-test - timer command ownership and collection + * + * Copyright (C) 2026 Network UPS Tools project + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include "config.h" + +/* Include the implementation to test private command ownership. */ +int upssched_main(int argc, char **argv); +#define main upssched_main +#include "upssched.c" +#undef main + +int main(int argc, char **argv) +{ + command_t *child; + int count = 0, attempts; + static char *script[3]; + + if (argc == 3 && !strcmp(argv[1], "--command")) { + if (!strcmp(argv[2], "slow")) { + sleep(8); + } + return 0; + } + + script[0] = argv[0]; + script[1] = (char *)"--command"; + script[2] = NULL; + cmdscript_argv = script; + cmdscript_argc = 2; + cmdscript_concat = argv[0]; + + exec_cmd("slow", 1); + exec_cmd("fast", 1); + for (attempts = 0; attempts < 3; attempts++) { + sleep(1); + reap_commands(); + count = 0; + for (child = commands; child; child = child->next) { + count++; + } + if (count == 1) { + break; + } + } + if (count != 1 || strcmp(commands->name, "slow")) { + fprintf(stderr, "FAIL: fast command must finish while slow command runs\n"); + return 1; + } + for (attempts = 0; commands && attempts < 10; attempts++) { + sleep(1); + reap_commands(); + } + if (commands) { + fprintf(stderr, "FAIL: completed commands must be collected\n"); + return 1; + } + puts("PASS: independent timer command execution and collection"); + return 0; +}