Fix crash in audio source (#3)

Pulse Audio source is currently the only source that has its own thread
to poll for changes. Embedded Lua is not thread safe so at some cases
publishing audio source changes on that thread caused the program to
crash. This refactors to make sure that publishing of source states to
Lua always happens on main thread.

src/DateTimeSources.cpp

@@ -17,7 +17,9 @@ std::shared_ptr<DateSource> DateSource::Create() {
 }
 
 void DateSource::Evaluate() {
-    // TODO: Check dirty
+    // TODO: To save some redraws the redraw flag should check if date has changed since last draw
+    m_published = true;  // No need to publish
+    m_drawn = false;
 }
 
 std::shared_ptr<TimeSource> TimeSource::Create(MainLoop& mainLoop,
@@ -53,7 +55,8 @@ bool TimeSource::OnRead() {
         // Either block or no events
         return false;
     }
-    m_sourceDirtyFlag = true;
+    m_published = true;  // No need to publish
+    m_drawn = false;
     m_dateSource->Evaluate();
-    return m_sourceDirtyFlag;
+    return !m_drawn;
 }

src/DateTimeSources.h

@@ -8,6 +8,7 @@ class DateSource : public Source {
     static std::shared_ptr<DateSource> Create();
     void Evaluate();
     virtual ~DateSource() {}
+    void Publish(const std::string_view, ScriptContext&) override {}
 
    private:
     DateSource() {}
@@ -19,6 +20,7 @@ class TimeSource : public Source, public IoHandler {
                                               std::shared_ptr<DateSource> dateSource);
     virtual ~TimeSource();
     virtual bool OnRead() override;
+    void Publish(const std::string_view, ScriptContext&) override {}
 
    private:
     TimeSource(int fd, std::shared_ptr<DateSource> dateSource)

src/MainLoop.cpp

@@ -52,10 +52,8 @@ void MainLoop::Run() {
                 }
             }
         }
-        if (anyDirty) {
-            for (auto& handler : m_batchHandlers) {
-                handler->OnBatchProcessed();
-            }
+        if (anyDirty && m_batchHandler) {
+            m_batchHandler->OnBatchProcessed();
         }
     } while (m_polls.size() > 0);
 }
@@ -66,8 +64,11 @@ void MainLoop::Register(int fd, const std::string_view name, std::shared_ptr<IoH
     spdlog::debug("Registering {} in main loop for fd {}", name, fd);
 }
 
-void MainLoop::RegisterBatchHandler(std::shared_ptr<IoBatchHandler> ioBatchHandler) {
-    m_batchHandlers.push_back(ioBatchHandler);
+void MainLoop::RegisterBatchHandler(std::shared_ptr<IoBatchHandler> batchHandler) {
+    if (m_batchHandler) {
+        spdlog::error("Only one batch handler supported");
+    }
+    m_batchHandler = batchHandler;
 }
 
 void MainLoop::Wakeup() {

src/MainLoop.h

@@ -38,5 +38,5 @@ class MainLoop {
     std::mutex m_wakupMutex;
     std::vector<pollfd> m_polls;
     std::map<int, std::shared_ptr<IoHandler>> m_handlers;
-    std::vector<std::shared_ptr<IoBatchHandler>> m_batchHandlers;
+    std::shared_ptr<IoBatchHandler> m_batchHandler;
 };

src/Manager.cpp

@@ -3,16 +3,8 @@
 #include "spdlog/spdlog.h"
 #include "src/Registry.h"
 
-std::shared_ptr<Manager> Manager::Create(std::shared_ptr<Registry> registry,
-                                         std::string_view sourceName, MainLoop& mainLoop,
-                                         std::unique_ptr<Sources> sources,
-                                         std::shared_ptr<ScriptContext> scriptContext) {
-    auto manager = std::shared_ptr<Manager>(
-        new Manager(registry, sourceName, std::move(sources), scriptContext));
-    // Register as a source
-    manager->m_sources->Register(sourceName, manager);
-    // Register source batch handler
-    mainLoop.RegisterBatchHandler(manager);
+std::shared_ptr<Manager> Manager::Create(std::shared_ptr<Registry> registry) {
+    auto manager = std::shared_ptr<Manager>(new Manager(registry));
     // Register click handler
     if (!registry->seat) {
         spdlog::error("No seat in registry");
@@ -29,39 +21,32 @@ void Manager::ClickSurface(wl_surface* surface, int x, int y) {
 }
 
 void Manager::OnBatchProcessed() {
+    // Always publish sources
+    m_sources->PublishAll();
     // No need to redraw when not visible and not in transition
     if (!m_visibilityChanged && !m_isVisible) return;
-
     spdlog::debug("Processing batch of dirty sources");
     if (m_visibilityChanged) {
         m_visibilityChanged = false;
         if (m_isVisible) {
-            m_sources->DirtyAll();
+            m_sources->ForceRedraw();
         } else {
             m_registry->BorrowOutputs().Hide(*m_registry);
             return;
         }
     }
     m_registry->BorrowOutputs().Draw(*m_registry, *m_sources);
-    m_sources->CleanAll();
-}
-
-void Manager::Publish(const Displays& displays) {
-    m_scriptContext->Publish(m_sourceName, displays);
-    m_sourceDirtyFlag = true;
-    OnBatchProcessed();
+    m_sources->SetAllDrawn();
 }
 
 void Manager::Hide() {
     m_isVisible = false;
     m_visibilityChanged = true;
-    m_sourceDirtyFlag = true;
     OnBatchProcessed();
 }
 
 void Manager::Show() {
     m_isVisible = true;
     m_visibilityChanged = true;
-    m_sourceDirtyFlag = true;
     OnBatchProcessed();
 }

src/Manager.h

@@ -5,35 +5,26 @@
 #include "src/ScriptContext.h"
 #include "src/Sources.h"
 
-class Manager : public IoBatchHandler, public Source {
+class Manager : public IoBatchHandler {
    public:
-    static std::shared_ptr<Manager> Create(std::shared_ptr<Registry> registry,
-                                           std::string_view sourceName, MainLoop& mainLoop,
-                                           std::unique_ptr<Sources> sources,
-                                           std::shared_ptr<ScriptContext> scriptContext);
+    static std::shared_ptr<Manager> Create(std::shared_ptr<Registry> registry);
+    void SetSources(std::unique_ptr<Sources> sources) { m_sources = std::move(sources); }
     virtual ~Manager() {}
-    // When a batch of IO events has been processed and sources are potentially dirty
+    // When a batch of IO events has been processed and sources needs to be published and/or needs
+    // to redrawn
     void OnBatchProcessed() override;
 
-    // Used by compositor implementation
+    // Compositors tells manager when the overlays should be visible
     void Show();
     void Hide();
-    void Publish(const Displays& displays);
 
     void ClickSurface(wl_surface* surface, int x, int y);
 
    private:
-    Manager(std::shared_ptr<Registry> registry, std::string_view sourceName,
-            std::unique_ptr<Sources> sources, std::shared_ptr<ScriptContext> scriptContext)
-        : m_registry(registry),
-          m_sourceName(sourceName),
-          m_sources(std::move(sources)),
-          m_scriptContext(scriptContext) {}
+    Manager(std::shared_ptr<Registry> registry) : m_registry(registry) {}
 
     std::shared_ptr<Registry> m_registry;
-    std::string m_sourceName;
     bool m_isVisible;
     bool m_visibilityChanged;
     std::unique_ptr<Sources> m_sources;
-    std::shared_ptr<ScriptContext> m_scriptContext;
 };

src/NetworkSource.cpp

@@ -30,8 +30,7 @@ static char *get_ip_str(const struct sockaddr *sa, char *s, size_t maxlen) {
 
     return s;
 }
-std::shared_ptr<NetworkSource> NetworkSource::Create(std::string_view name, MainLoop &mainLoop,
-                                                     std::shared_ptr<ScriptContext> scriptContext) {
+std::shared_ptr<NetworkSource> NetworkSource::Create(MainLoop &mainLoop) {
     auto sock = socket(PF_INET, SOCK_DGRAM, 0);
     if (sock < 0) {
         return nullptr;
@@ -48,8 +47,8 @@ std::shared_ptr<NetworkSource> NetworkSource::Create(std::string_view name, Main
         spdlog::error("Failed to set timer: {}", strerror(errno));
         return nullptr;
     }
-    auto source = std::shared_ptr<NetworkSource>(new NetworkSource(name, sock, fd, scriptContext));
-    mainLoop.Register(fd, name, source);
+    auto source = std::shared_ptr<NetworkSource>(new NetworkSource(sock, fd));
+    mainLoop.Register(fd, "NetworkSource", source);
     return source;
 }
 
@@ -90,10 +89,9 @@ void NetworkSource::ReadState() {
         network.address = get_ip_str(addr, ip, sizeof(ip));
         networks.push_back(std::move(network));
     }
-    m_sourceDirtyFlag = true;  // m_networks != networks;
-    if (m_sourceDirtyFlag) {
+    if (m_networks != networks) {
+        m_drawn = m_published = false;
         m_networks = networks;
-        m_scriptContext->Publish(m_name, m_networks);
     }
 }
 
@@ -105,6 +103,11 @@ bool NetworkSource::OnRead() {
         // Either block or no events
         return false;
     }
-    ReadState();
-    return m_sourceDirtyFlag;
+    return !m_published;
+}
+
+void NetworkSource::Publish(const std::string_view sourceName, ScriptContext &scriptContext) {
+    if (m_published) return;
+    scriptContext.Publish(sourceName, m_networks);
+    m_published = true;
 }

src/NetworkSource.h

@@ -8,19 +8,16 @@
 
 class NetworkSource : public Source, public IoHandler {
    public:
-    static std::shared_ptr<NetworkSource> Create(std::string_view name, MainLoop& mainLoop,
-                                                 std::shared_ptr<ScriptContext> scriptContext);
+    static std::shared_ptr<NetworkSource> Create(MainLoop& mainLoop);
     void Initialize();
     void ReadState();
     virtual bool OnRead() override;
+    void Publish(const std::string_view sourceName, ScriptContext& scriptContext) override;
     virtual ~NetworkSource() { close(m_timerfd); }
 
    private:
-    NetworkSource(std::string_view name, int socket, int timerfd,
-                  std::shared_ptr<ScriptContext> scriptContext)
-        : m_name(name), m_socket(socket), m_timerfd(timerfd), m_scriptContext(scriptContext) {}
+    NetworkSource(int socket, int timerfd) : m_socket(socket), m_timerfd(timerfd) {}
 
-    const std::string m_name;
     int m_socket;
     int m_timerfd;
     std::shared_ptr<ScriptContext> m_scriptContext;

src/Output.cpp

@@ -132,7 +132,7 @@ void Outputs::Draw(const Registry &registry, const Sources &sources) {
     for (const auto &panelConfig : m_config->panels) {
         bool dirty = false;
         for (const auto &widgetConfig : panelConfig.widgets) {
-            if (sources.IsDirty(widgetConfig.sources)) {
+            if (sources.NeedsRedraw(widgetConfig.sources)) {
                 dirty = true;
                 break;
             }

src/PowerSource.cpp

@@ -8,8 +8,7 @@
 #include <optional>
 #include <string>
 
-std::shared_ptr<PowerSource> PowerSource::Create(std::string_view name, MainLoop& mainLoop,
-                                                 std::shared_ptr<ScriptContext> scriptContext) {
+std::shared_ptr<PowerSource> PowerSource::Create(MainLoop& mainLoop) {
     // Use non blocking to make sure we never hang on read
     auto fd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC | TFD_NONBLOCK);
     if (fd == -1) {
@@ -23,15 +22,14 @@ std::shared_ptr<PowerSource> PowerSource::Create(std::string_view name, MainLoop
         spdlog::error("Failed to set timer: {}", strerror(errno));
         return nullptr;
     }
-    auto source = std::shared_ptr<PowerSource>(new PowerSource(name, fd, scriptContext));
-    mainLoop.Register(fd, name, source);
+    auto source = std::shared_ptr<PowerSource>(new PowerSource(fd));
+    mainLoop.Register(fd, "PowerSource", source);
     return source;
 }
 
 bool PowerSource::Initialize() {
-    // Publish initial value to make sure that something is published
-    m_scriptContext->Publish(m_name,
-                             PowerState{.IsPluggedIn = false, .IsCharging = false, .Capacity = 0});
+    m_drawn = m_published = false;
+    m_sourceState = PowerState{.IsPluggedIn = false, .IsCharging = false, .Capacity = 0};
     // TODO: Probe that these exists
     m_ac = "/sys/class/power_supply/AC/online";
     // TODO: Always BAT0?
@@ -91,12 +89,11 @@ void PowerSource::ReadState() {
     if (maybeString) {
         state.IsPluggedIn = std::stoi(*maybeString) != 0;
     }
-    m_sourceDirtyFlag = state != m_sourceState;
-    if (m_sourceDirtyFlag) {
-        m_sourceState = state;
-        m_scriptContext->Publish(m_name, m_sourceState);
+    if (state != m_sourceState) {
         spdlog::info("Power status changed, capacity {}, charging {}, plugged in {}",
                      state.Capacity, state.IsCharging, state.IsPluggedIn);
+        m_sourceState = state;
+        m_drawn = m_published = false;
     }
 }
 
@@ -111,5 +108,11 @@ bool PowerSource::OnRead() {
         return false;
     }
     ReadState();
-    return m_sourceDirtyFlag;
+    return !m_published;
+}
+
+void PowerSource::Publish(const std::string_view sourceName, ScriptContext& scriptContext) {
+    if (m_published) return;
+    scriptContext.Publish(sourceName, m_sourceState);
+    m_published = true;
 }

src/PowerSource.h

@@ -8,21 +8,18 @@
 
 class PowerSource : public Source, public IoHandler {
    public:
-    static std::shared_ptr<PowerSource> Create(std::string_view name, MainLoop& mainLoop,
-                                               std::shared_ptr<ScriptContext> scriptContext);
+    static std::shared_ptr<PowerSource> Create(MainLoop& mainLoop);
     bool Initialize();
     void ReadState();
     virtual bool OnRead() override;
+    void Publish(const std::string_view sourceName, ScriptContext& scriptContext) override;
     virtual ~PowerSource();
 
    private:
-    PowerSource(std::string_view name, int fd, std::shared_ptr<ScriptContext> scriptContext)
-        : m_name(name), m_timerfd(fd), m_scriptContext(scriptContext) {}
-    const std::string m_name;
+    PowerSource(int fd) : m_timerfd(fd) {}
     std::filesystem::path m_batteryCapacity;
     std::filesystem::path m_batteryStatus;
     std::filesystem::path m_ac;
     int m_timerfd;
-    std::shared_ptr<ScriptContext> m_scriptContext;
     PowerState m_sourceState;
 };

src/PulseAudioSource.cpp

@@ -41,9 +41,7 @@ static void on_subscribe(pa_context* ctx, pa_subscription_event_type_t event_and
     }
 }
 
-std::unique_ptr<PulseAudioSource> PulseAudioSource::Create(
-    std::string_view name, std::shared_ptr<MainLoop> zenMainloop,
-    std::shared_ptr<ScriptContext> scriptContext) {
+std::unique_ptr<PulseAudioSource> PulseAudioSource::Create(std::shared_ptr<MainLoop> zenMainloop) {
     auto mainloop = pa_threaded_mainloop_new();
     if (!mainloop) return nullptr;
     pa_threaded_mainloop_lock(mainloop);
@@ -59,7 +57,7 @@ std::unique_ptr<PulseAudioSource> PulseAudioSource::Create(
         return nullptr;
     }
     auto backend = std::unique_ptr<PulseAudioSource>(
-        new PulseAudioSource(name, zenMainloop, mainloop, scriptContext, api, context));
+        new PulseAudioSource(zenMainloop, mainloop, api, context));
     // From now on the backend will free on error
 
     if (pa_context_connect(context, nullptr, PA_CONTEXT_NOFAIL, nullptr) < 0) {
@@ -144,10 +142,20 @@ void PulseAudioSource::OnSinkChange(const pa_sink_info* info) {
                 break;
         }
     }
-    if (newState == m_sourceState) return;
-    spdlog::debug("Audio source is dirty");
-    m_sourceDirtyFlag = true;
-    m_sourceState = newState;
-    m_scriptContext->Publish(m_name, m_sourceState);
-    m_zenMainloop->Wakeup();
+    {
+        std::lock_guard<std::mutex> lock(m_mutex);
+        if (newState == m_sourceState) return;
+        spdlog::debug("Audio source is dirty");
+        m_drawn = m_published = false;
+        m_sourceState = newState;
+        m_zenMainloop->Wakeup();
+    }
+}
+
+// This is invoked on main thread. Can not publish on another thread
+void PulseAudioSource::Publish(const std::string_view sourceName, ScriptContext& scriptContext) {
+    std::lock_guard<std::mutex> lock(m_mutex);
+    if (m_published) return;
+    scriptContext.Publish(sourceName, m_sourceState);
+    m_published = true;
 }