feat(multisync): apply sync destination changes without restarting fppd

Ticking a per-remote Unicast checkbox on the MultiSync page, or toggling any
of the send-method checkboxes, flagged fppd as needing a restart. It never
had to: fppd already re-reads the settings file whenever the web UI writes it
(FileMonitor in fppd.cpp), and settings.cpp already has a listener mechanism
for reacting to individual keys. The only thing missing was rebuilding the
destination list those settings feed, which OpenControlSockets() did once at
startup and never again.

Split that parsing out into ReloadSyncDestinations() and register it against
MultiSyncRemotes, MultiSyncExtraRemotes and the three send-method settings.
Hostnames are resolved into a local vector first, since getaddrinfo() is an
unbounded lookup and the send path takes m_socketLock on the output thread
every frame; the sockaddr and mmsghdr vectors are then swapped in together
under that lock, because each mmsghdr points at its own element of the
sockaddr vector and the pair must never be observed half-updated. The rebuild
holds the same lock UpdateUnicastDestinations() uses, so the "all known
remotes" list can't read the static list for its dedupe set mid-swap. The
three send-method flags become atomic: the output thread reads them without
holding anything. Listeners are unregistered in ShutdownSync(), which is
idempotent because the destructor calls it again at static-destruction time;
that also acts as a barrier, since unregistering takes the listener list's
write lock that the firing loop holds while a callback runs.

Two bugs found in the parsing while moving it:

- Hostnames have never worked. A 2020 refactor rewrote "does this contain a
  letter" as `find_if(...) == s.end()`, which is the opposite test, so
  hostnames took the inet_addr() path, came back INADDR_NONE, and were then
  installed as a destination of 255.255.255.255 -- every entry that wasn't
  already a dotted quad silently broadcast its sync packets. An unparseable
  address is now rejected and logged instead of becoming that destination.
- Clearing the remote list stored a literal pair of quote characters, since
  the page PUTs the value as a JSON string. That parsed as a hostname and
  cost a full DNS timeout (~4s) on every load. Tokens are unquoted first.

Verified on a single-core ARM player. Sync destinations, with tcpdump running
across a settings change mid-playback and no restart: packets to the first
loopback address stop, the reload logs, packets to the second start, all
within 250ms and all under one fppd pid -- the new destination starts and the
old one stops. Hostname handling: a remote entered as "localhost" now sends to
127.0.0.1 rather than the broadcast address, and a malformed dotted quad
yields zero destinations instead of one bad one. In headless Chrome, clicking
a per-remote Unicast checkbox raises no restart banner in 60 samples over 15s
(one flash before this change) and the checkbox survives every table re-render.

Fixes #2834

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: dkulp

src/MultiSync.cpp

@@ -309,6 +309,23 @@ int MultiSync::Init(void) {
     };
     NetworkMonitor::INSTANCE.registerCallback(f);
 
+    // The sync send methods and the unicast remote list are picked up live.
+    // fppd already re-reads /media/settings whenever the web UI writes it
+    // (FileMonitor in fppd.cpp), so all that was missing was rebuilding the
+    // destination list these settings feed -- editing the remote list on the
+    // MultiSync page no longer needs an fppd restart to take effect.  The
+    // control socket itself is unaffected by any of them; if it isn't open yet
+    // (MultiSync disabled, or nothing has needed it), OpenControlSockets()
+    // reads the current settings when it does open.
+    for (const char* s : { "MultiSyncRemotes", "MultiSyncExtraRemotes",
+                           "MultiSyncBroadcast", "MultiSyncMulticast", "MultiSyncUnicast" }) {
+        registerSettingsListener("MultiSync", s, [this](const std::string& v) {
+            if (m_controlSock >= 0) {
+                ReloadSyncDestinations();
+            }
+        });
+    }
+
     return 1;
 }
 
@@ -1890,6 +1907,21 @@ void MultiSync::removeMultiSyncPlugin(MultiSyncPlugin* p) {
 void MultiSync::ShutdownSync(void) {
     LogDebug(VB_SYNC, "ShutdownSync()\n");
 
+    // Idempotent, like WLEDAudioSync::Cleanup(): fppd's shutdown calls this and
+    // ~MultiSync() calls it again at static-destruction time, by which point the
+    // global SettingsConfig may already be gone -- locking its destroyed mutex
+    // would throw out of a noexcept destructor.  Unregistering also acts as a
+    // barrier: unregisterSettingsListener() takes the listener list's write
+    // lock, which the firing loop holds for reading while a callback runs, so it
+    // cannot return while ReloadSyncDestinations() is still in flight on the
+    // settings-reload thread.
+    if (!m_settingsListenersRemoved.exchange(true)) {
+        for (const char* s : { "MultiSyncRemotes", "MultiSyncExtraRemotes",
+                               "MultiSyncBroadcast", "MultiSyncMulticast", "MultiSyncUnicast" }) {
+            unregisterSettingsListener("MultiSync", s);
+        }
+    }
+
     for (auto a : m_plugins) {
         a->ShutdownSync();
     }
@@ -1966,6 +1998,22 @@ int MultiSync::OpenControlSockets() {
         return 0;
     }
 
+    ReloadSyncDestinations();
+
+    FillInInterfaces();
+
+    return 1;
+}
+
+void MultiSync::ReloadSyncDestinations() {
+    // Same lock UpdateUnicastDestinations() uses, for the same reason: the
+    // resolve step below is slow and runs without m_socketLock, so two
+    // overlapping reloads could otherwise finish out of order and leave the
+    // older result in place.  Holding it also keeps UpdateUnicastDestinations()
+    // from reading m_destAddr for its dedupe set mid-swap.  Released before the
+    // UpdateUnicastDestinations() call at the end, which takes it itself.
+    std::unique_lock<std::mutex> updateLock(m_unicastUpdateLock);
+
     std::string remotesString = getSetting("MultiSyncRemotes");
     std::string extraRemotes = getSetting("MultiSyncExtraRemotes");
     if (extraRemotes != "") {
@@ -1981,36 +2029,51 @@ int MultiSync::OpenControlSockets() {
     std::set<std::string> remotes;
     for (auto& token : tokens) {
         TrimWhiteSpace(token);
+        // The web UI PUTs this setting as a JSON string, so an empty list is
+        // stored as a literal pair of quote characters rather than as an empty
+        // value.  Left in, that becomes a "hostname" of "" and every reload
+        // spends a full DNS timeout (~4s, on the settings-reload thread)
+        // failing to resolve it.
+        while (token.size() >= 2 && token.front() == '"' && token.back() == '"') {
+            token = token.substr(1, token.size() - 2);
+            TrimWhiteSpace(token);
+        }
         if (token != "") {
             remotes.insert(token);
         }
     }
 
-    if (getSettingInt("MultiSyncBroadcast")) {
-        m_sendBroadcast = true;
-    }
-
-    if (getSettingInt("MultiSyncMulticast")) {
-        m_sendMulticast = true;
-    }
-    if (getSettingInt("MultiSyncUnicast")) {
-        m_sendUnicast = true;
-    }
-    if (remotesString == "" && !m_sendBroadcast && !m_sendMulticast && !m_sendUnicast && m_multiSyncEnabled) {
+    // Assign rather than |=: this runs again on every settings change, so a
+    // method the user just turned off has to go back to false.
+    bool sendBroadcast = getSettingInt("MultiSyncBroadcast") != 0;
+    bool sendMulticast = getSettingInt("MultiSyncMulticast") != 0;
+    bool sendUnicast = getSettingInt("MultiSyncUnicast") != 0;
+    if (remotesString == "" && !sendBroadcast && !sendMulticast && !sendUnicast && m_multiSyncEnabled) {
         // No explicit remotes or send method configured; default to multicast.
-        m_sendMulticast = true;
+        sendMulticast = true;
     }
 
+    // Resolve into a local list first: getaddrinfo() below is an unbounded
+    // network lookup and must not run while m_socketLock is held, since that
+    // lock is taken by the send path on the output thread every frame.
+    std::vector<struct sockaddr_in> newAddrs;
     for (auto& s : remotes) {
         LogDebug(VB_SYNC, "Setting up Remote Sync for %s\n", s.c_str());
         struct sockaddr_in newRemote;
+        memset(&newRemote, 0, sizeof(newRemote));
 
         newRemote.sin_family = AF_INET;
         newRemote.sin_port = htons(FPP_CTRL_PORT);
 
-        bool isAlpha = std::find_if(s.begin(), s.end(), [](char c) { return (isalpha(c) || (c == ' ')); }) == s.end();
+        // A letter (or a space) means this is a hostname and has to be resolved;
+        // anything else is a dotted-quad to parse directly.  The test used to
+        // read `... == s.end()`, i.e. "contains no letters", which is backwards:
+        // hostnames took the inet_addr() path, came back INADDR_NONE, and were
+        // then installed as a destination of 255.255.255.255 -- so every entry
+        // that wasn't already an IP address quietly broadcast its sync packets.
+        bool isHostname = std::find_if(s.begin(), s.end(), [](char c) { return (isalpha(c) || (c == ' ')); }) != s.end();
         bool valid = true;
-        if (isAlpha) {
+        if (isHostname) {
             // Use the reentrant getaddrinfo() rather than gethostbyname(), which
             // shares a single static hostent across the process.
             struct addrinfo hints{};
@@ -2028,33 +2091,58 @@ int MultiSync::OpenControlSockets() {
             }
         } else {
             newRemote.sin_addr.s_addr = inet_addr(s.c_str());
+            if (newRemote.sin_addr.s_addr == INADDR_NONE) {
+                LogErr(VB_SYNC, "Error parsing Remote IP address: %s\n", s.c_str());
+                valid = false;
+            }
         }
         if (valid) {
-            m_destAddr.push_back(newRemote);
+            newAddrs.push_back(newRemote);
         }
     }
-    for (int x = 0; x < m_destAddr.size(); x++) {
-        struct mmsghdr msg;
-        memset(&msg, 0, sizeof(msg));
 
-        msg.msg_hdr.msg_name = &m_destAddr[x];
-        msg.msg_hdr.msg_namelen = sizeof(sockaddr_in);
-        msg.msg_hdr.msg_iov = &m_destIovec;
-        msg.msg_hdr.msg_iovlen = 1;
-        msg.msg_len = 0;
-        m_destMsgs.push_back(msg);
-    }
-
-    LogDebug(VB_SYNC, "%d Remote Sync systems configured\n",
-             m_destAddr.size());
-    FillInInterfaces();
+    m_sendBroadcast = sendBroadcast;
+    m_sendMulticast = sendMulticast;
+    m_sendUnicast = sendUnicast;
 
-    // Seed the "all known remotes" unicast list from anything already known.
-    // (Typically empty at startup; it fills in as remotes are discovered.)
-    if (m_sendUnicast) {
+    {
+        // Swap the whole list in at once.  Each mmsghdr points at its own
+        // element of m_destAddr, so the two vectors must be rebuilt together
+        // and never observed half-updated by SendControlPacketViaMsgs().
+        std::unique_lock<std::mutex> lock(m_socketLock);
+        m_destAddr = std::move(newAddrs);
+        m_destMsgs.clear();
+        m_destMsgs.reserve(m_destAddr.size());
+        for (size_t x = 0; x < m_destAddr.size(); x++) {
+            struct mmsghdr msg;
+            memset(&msg, 0, sizeof(msg));
+
+            msg.msg_hdr.msg_name = &m_destAddr[x];
+            msg.msg_hdr.msg_namelen = sizeof(sockaddr_in);
+            msg.msg_hdr.msg_iov = &m_destIovec;
+            msg.msg_hdr.msg_iovlen = 1;
+            msg.msg_len = 0;
+            m_destMsgs.push_back(msg);
+        }
+        LogDebug(VB_SYNC, "%d Remote Sync systems configured\n",
+                 (int)m_destAddr.size());
+
+        if (!sendUnicast) {
+            // "Send to ALL KNOWN remotes" is off, so drop that list rather than
+            // leaving a stale copy behind for the next time it is turned on.
+            m_unicastDestMsgs.clear();
+            m_unicastDestAddr.clear();
+        }
+    }
+    updateLock.unlock();
+
+    // Rebuild the "all known remotes" list too: it dedupes itself against
+    // m_destAddr, which just changed.  Must run with both locks released --
+    // UpdateUnicastDestinations() takes m_unicastUpdateLock, then m_systemsLock,
+    // then m_socketLock, itself.
+    if (sendUnicast) {
         UpdateUnicastDestinations();
     }
-    return 1;
 }
 
 void MultiSync::SendControlPacketViaMsgs(std::vector<struct mmsghdr>& msgs, struct iovec& iovec, void* outBuf, int len) {
@@ -2181,7 +2269,8 @@ void MultiSync::UpdateUnicastDestinations() {
     // m_systemsLock -> m_socketLock order used by Ping().
     std::unique_lock<std::mutex> lock(m_socketLock);
     // Skip any address already covered by the statically-configured remote list
-    // (m_destAddr, built once in OpenControlSockets and immutable thereafter) so
+    // (m_destAddr, rebuilt by ReloadSyncDestinations(), which holds the same
+    // m_unicastUpdateLock we hold here while it swaps) so
     // a remote that is both individually selected for unicast AND picked up by
     // "all known remotes" only receives each packet once.  The same set also
     // dedupes the all-known list against itself (e.g. a remote known under both

src/MultiSync.h

@@ -346,6 +346,14 @@ class MultiSync {
     // currently-known remote systems that support unicast.  Only used when the
     // MultiSyncUnicast ("send to ALL known remotes") setting is enabled.
     void UpdateUnicastDestinations();
+    // Re-read the sync send methods (MultiSyncBroadcast/Multicast/Unicast) and
+    // the statically-configured unicast remote list (MultiSyncRemotes plus
+    // MultiSyncExtraRemotes) from the settings and swap the resulting
+    // destination list into place.  Called once from OpenControlSockets() and
+    // again from a settings listener whenever any of those settings change, so
+    // editing the remote list on the MultiSync page takes effect immediately
+    // instead of requiring an fppd restart.
+    void ReloadSyncDestinations();
     bool FillInInterfaces();
     bool RemoveInterface(const std::string& interface);
 
@@ -377,14 +385,21 @@ class MultiSync {
     std::map<std::string, std::string> m_configuredOutputRanges;
 
     std::map<std::string, NetInterfaceInfo> m_interfaces;
-    bool m_sendMulticast;
-    bool m_sendBroadcast;
-    bool m_sendUnicast = false;
+    // Which send methods are active.  Read on the output thread from
+    // SendControlPacket() without any lock and rewritten by
+    // ReloadSyncDestinations() on the settings-reload thread, hence atomic.
+    std::atomic<bool> m_sendMulticast;
+    std::atomic<bool> m_sendBroadcast;
+    std::atomic<bool> m_sendUnicast = false;
 
     int m_broadcastSock;
     int m_controlSock;
     int m_receiveSock;
 
+    // Guards the one-shot listener teardown in ShutdownSync(); see the comment
+    // there for why the second call has to be a no-op.
+    std::atomic<bool> m_settingsListenersRemoved{ false };
+
     std::string m_hostname;
     struct sockaddr_in m_srcAddr;
     struct sockaddr_in m_receiveSrcAddr;
@@ -414,6 +429,9 @@ class MultiSync {
 
     float m_remoteOffset;
 
+    // Statically-configured unicast remotes (MultiSyncRemotes plus
+    // MultiSyncExtraRemotes).  Rebuilt by ReloadSyncDestinations() whenever
+    // those settings change.  Guarded by m_socketLock (same as the send path).
     struct iovec m_destIovec;
     std::vector<struct mmsghdr> m_destMsgs;
     std::vector<struct sockaddr_in> m_destAddr;

www/multisync.php

@@ -3559,17 +3559,15 @@ function ensureSomeSyncMethod() {
                 if (!r.ok) throw new Error(r.status);
                 settings['MultiSyncRemotes'] = remotes;
                 if (verbose) {
+                    // No restart flag: fppd re-reads /media/settings when it
+                    // changes and MultiSync rebuilds its unicast destination
+                    // list from this setting live (MultiSync::ReloadSyncDestinations).
                     if (remotes == "") {
-                        $.jGrowl("Remote List Cleared.  You must restart fppd for the changes to take effect.", { themeState: 'success' });
+                        $.jGrowl("Remote List Cleared.", { themeState: 'success' });
                     } else {
-                        $.jGrowl("Remote List set to: '" + remotes + "'.  You must restart fppd for the changes to take effect.", { themeState: 'success' });
+                        $.jGrowl("Remote List set to: '" + remotes + "'.", { themeState: 'success' });
                     }
                 }
-                //Mark FPPD as needing restart
-                SetRestartFlag(2);
-                settings['restartFlag'] = 2;
-                //Get the resart banner showing
-                CheckRestartRebootFlags();
                 validateMultiSyncSettings();
             } catch {
                 DialogError("Save Remotes", "Save Failed");

www/settings.json

@@ -2365,7 +2365,6 @@
 			"description": "Send MultiSync to ALL remotes via Broadcast",
 			"tip": "Use a network broadcast to send MultiSync messages to all remotes.",
 			"type": "checkbox",
-			"restart": 2,
 			"fppModes": [
 				"player"
 			],
@@ -2379,7 +2378,6 @@
 			"description": "Send MultiSync to ALL KNOWN remotes via Unicast",
 			"tip": "Unicast MultiSync messages individually to every known FPP remote that is running in Remote mode.  Remotes are added to the list automatically as they are discovered (via mDNS or the normal FPP discovery).  Non-FPP devices (WLED, ESPixelStick, Falcon controllers, etc.) and FPP instances not in Remote mode are skipped.  Useful when multicast/broadcast is unreliable across your network.",
 			"type": "checkbox",
-			"restart": 2,
 			"default": 1,
 			"fppModes": [
 				"player"
@@ -2442,7 +2440,6 @@
 			"description": "MultiSync Unicast Discovery IPs (CSV list)",
 			"tip": "FPP will send unicast MultiSync discovery messages to the IPs in this list to allow discovery of devices which are unable to be discovered via FPP's normal multicast discovery.  This may include systems on remote networks or connected to certain routers which do not pass multicast packets between wired and wireless interfaces.",
 			"level": 1,
-			"restart": 2,
 			"size": 64,
 			"maxlength": 128,
 			"type": "text",
@@ -2495,7 +2492,6 @@
 			"description": "Send MultiSync to ALL remotes via Multicast (239.70.80.80)",
 			"tip": "Send MultiCast to 239.70.80.80 to send MultiSync messages to all remotes.",
 			"type": "checkbox",
-			"restart": 2,
 			"fppModes": [
 				"player"
 			],