Simplify main event loop, improve reactivity

[leana8959]

Config reload, Dbus messages, and socket IO are each done on a different
thread without polling.
They communicate with the event loop using the event loop proxy.

CONFIG and DBUS_CONN global no longer need unsafe to be used.
arcswap crate was added as dependency to make CONFIG a safe global.
To make CONFIG Send, image_block's ImageSurface cache is moved to
NotifyWindow. ImageSurface is a raw pointer under the hood and isn't
Send.
Dbus processing thread's timeout is removed as well.

idle_polling configuration is removed. Wired-notify now sleeps
completely when no notification is present. The polling only starts when
a notification is received, and wired-notify sleeps again when they are
all gone.

[Toqozz]

Wow! I didn't expect you to take a shot at this. Happy for it though.

Firstly, I'd strongly prefer leaving the global variables for CONFIG and DBUS_CONN as uncomplicated as possible.

For DBUS_CONN, I see no reason for this to change. Its access is already completely safe -- process() on the dbus thread, and send() from the main thread. Adding locks and such just complicates the program with no tangible benefit.

The story is less clear for CONFIG. It seems some genuinely unsafe usage has crept in, as we now access CONFIG from the dbus thread, which is unsafe (since the config may be reloaded while it's being used there).

Ideally I would prefer we just restrict config access to the main thread -- this was the original contract and the way the program was built. Unfortunately, the dbus thread now uses the config to figure out icon paths, and I actually would like to keep that process on the dbus thread, because loading the images themselves can be quite slow.

Still, I feel icky making such a large change just so that we can see a Vec<String> (which hardly changes) from another thread. I also really don't like the image_block changes, so maybe arcswap isn't helping us that much here?

Do you hate the idea of using a global structure guarded by a lock?

// dbus.rs
static ICON_THEME_CHAIN: RwLock<Vec<String>> = RwLock::new(Vec::new());
// ...
let chain = ICON_THEME_CHAIN.read().unwrap().clone();
let app_image = icons::resolve_icon_path(&app_icon, &chain)
    .and_then(|p| image_from_path(p.to_str().unwrap()));

// On config reload.
*ICON_THEME_CHAIN.write().unwrap() = Config::get().icon_theme_chain.clone();

P.S.: I am aware that Config is also used in the dbus thread for timeout stuff and whitespace trimming, but my idea here is that we would just have some apply_config() function that applies those rules on the main thread after we receive the Notification struct from the dbus thread.

---

I like the stuff you've done with the event loop on the surface, but will have to take a closer inspection at some point, since problems here can be very subtle.

---

For the CLI socket stuff, this is loosely the shape I'm imagining, but needs some changes.

I would really like the CLI to communicate with the main wired process through actual structured messages as early as possible, rather than strings.

When the socket has data, we should immediately parse it into a type, so that the rest of the program doesn't need to worry about parsing issues.

enum CLICommand {
    Drop(s32),
    Action(u32, u32),
    // ...
}

and handle_cli_command() should not have to dish out CLIError::Parse() anymore.

The same thing goes for sending data back to the socket.

So I think something like this:

1. Create some kind of socket thread. Have it create 2 mpsc::channel pairs, so that it has 2-way communication with the main thread.
2. Each loop, on the socket thread:
   • Consume all the data on the read socket.
   • Parse it into structured messages.
   • Send those messages to the wired process, via the channel Sender.
   • Then, separately...
   • Consume all the structured messages that have been sent back to us (right now, only dnd status message).
   • Encode them into strings, and send them along the write socket.

This makes it so that the boundary is very clear, and prevents details leaking into other parts of the code. We can see the consequence of not having the boundary clear enough in the case of the config, where now there is behaviour which depends on the boundary being broken, and it's annoying to fix.

The reason I didn't do it this way from the start is because the path was much shorter. Basically fire a message on the socket and forget. Now that we have 2-way communication, threads, etc, I'm feeling the need to make the boundary clearer.

[leana8959]

Thanks for the review and the reply!

For DBUS_CONN, I see no reason for this to change. Its access is already completely safe -- process() on the dbus thread, and send() from the main thread. Adding locks and such just complicates the program with no tangible benefit.

I'd suggest the contrary. As you said, wired has two threads accessing DBUS_CONN, given that I don't know (and don't think we should need to know) the implementation details of dbus, why not let the compiler guarantee that it is safe concurrently for us?

Given that dbus crate already provides a SyncConnection, a connection handle that is Sync (can be shared between threads), why not use it?
The lock on crossroads is due to SyncConnection requiring its method arguments to be Sync as well.

The story is less clear for CONFIG. It seems some genuinely unsafe usage has crept in, as we now access CONFIG from the dbus thread, which is unsafe (since the config may be reloaded while it's being used there).

I can confirm that I have managed to crash wired when adding a new layout. It died after the reload. This was before the refactor and I haven't tested it with the refactor. It might be due to existing layout's name being changed during the lifetime of a window.

Unfortunately, the dbus thread now uses the config to figure out icon paths, and I actually would like to keep that process on the dbus thread, because loading the images themselves can be quite slow. Still, I feel icky making such a large change just so that we can see a Vec (which hardly changes) from another thread.

Is there a way we can "push it up a level" to a outer structure that is stateful ?
I have done so for the open file handle, I moved it from the main thread to the manager. On each config reload, it would open the file again to get a fresh file handle.

I also really don't like the image_block changes, so maybe arcswap isn't helping us that much here?

I do see the advantage of putting the state in the image_block: each block in the window is self-contained and has its own state. I think the image_block refactor is ugly, but without it you have a shared global mutable state which is the config itself that you have to believe or prove to be correct when shared across threads, somehow, even in the presence of mutable C pointers. And that is not very nice either.
ArcSwap makes most stuff safe, but it can't gurantee that the mutable C pointer in Rect can be moved to another thread and then dropped in that thread safely (what "being Send" means, Rect is not Send). That is why I moved the image cache out.

I am aware that Config is also used in the dbus thread for timeout stuff and whitespace trimming, but my idea here is that we would just have some apply_config() function that applies those rules on the main thread after we receive the Notification struct from the dbus thread.

If we manage to make dbus no longer depend on the config, we can move config into manager as a static data. Then, the config watch would only need to tell the manager to reload the config by passing it a new one. Config would no longer need to be Send.

Do you hate the idea of using a global structure guarded by a lock?

I don't, I think it's convenient, which is why I didn't move the config to the manager.
It is also possible to resort back to unsafe if you want. I simply pushed the question "how remove unsafe as much as I can" to its conclusion and see if starting from here we can find a reasonable solution.

I like the stuff you've done with the event loop on the surface, but will have to take a closer inspection at some point, since problems here can be very subtle.

Nice! Well, there's no hurry. I have been using this branch of wired compiled to source to see if there are obvious overlooks.

I would really like the CLI to communicate with the main wired process through actual structured messages as early as possible, rather than strings.

That is also a good solution. Maybe we can use serde to do this?
It feels like a scope creep though, I wanted to simplify the main event loop for this PR, and then do that in another one.
If you think it would still be reviewable, I can give it a shot.

Create some kind of socket thread. Have it create 2 mpsc::channel pairs, so that it has 2-way communication with the main thread.

I think this is a good idea, but one pair should be enough. From the socket thread we can send messages to the event loop using the event loop proxy; from the event loop to the socket we can use one mpsc:channel pair. The proxy is better because it removes the delay, otherwise we would need to do polling.