Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor IPC to better facilitate scripting(?) use-cases of the niri-ipc crate #294

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sodiboo
Copy link
Contributor

@sodiboo sodiboo commented Apr 17, 2024

Based on work in #278.

This basically reimplements the way niri-ipc currently works, in a way that is not necessarily objectively better. I've marked this as a draft for now, given that i still need to figure out a couple things.

Some of the current issues that should be ironed out:

  1. Naming. One of the hard problems in computer science. What the heck are the names of HandleRequest and MsgRequest meant to be (in src/ipc/server.rs, src/ipc/client.rs, respectively)? Does the Request suffix for all request types make sense? Should it be a namespace?

  2. In the interest of not outright requiring all potential consumers of niri ipc to rewrite their logic, i've tried to keep an AnyRequest enum still functioning. This, complicates things, given that the response type is of course dependent on the request type, so i have an associated function decoder() whose sole purpose is capturing the state necessary to determine what kind of response we're going to read (and for the actual request types, that is a ZST). I would love to remove this, but i didn't yet because i'm not sure if we do want to support such a use case anyways (if a client wants, they could make their own request enum, after all)

  3. Make RequestMessage private? The RequestType tag is kind of an implementation detail. It's only necessary for serialization. Currently, it's exposed, though. A side effect of separating the types of each request and response is that you can no longer enumerate all requests on the client side, which i don't think is actually problematic. The motivation for doing this is to make a somewhat simpler IPC protocol, where we aren't relying on a representation of Rust sum types (because, the representation differs by whether it's a unit variant or similar). I'm not sure this is actually the best way to do it.

  4. Heavy reliance on serde_json::Value. Is this problematic? it feels dirty, but i'm also not exactly serializing them as real json strings, so it could be worse.

  5. Re-implement long-lived sockets (i.e. handle many requests per socket)


With these changes, i've taken care not to alter the behaviour of the niri msg utility. The actual logic behind it has only two changes:

  1. An out of bounds current_mode will result in Current mode: (invalid index) instead of exiting the entire operation and printing no useful information about outputs
  2. OutputRequest now returns a BTreeMap instead of a HashMap. This is because niri msg only ever sorted it before iterating, so it might as well come pre-sorted to save some overhead at that point.

Error handling may or may not be identical, but i did try to keep the behaviour as similar to before as possible.

And one great functionality of the code as is: All I/O logic happens in a distinct phase before any printing. In between them, i disable SIGPIPE, which is similar to what niri-ipc did before i ever touched it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant