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

feat: Add some way for awesome.spawn to set G_SPAWN stdio flags. #3865

Open
aarondill opened this issue Oct 20, 2023 · 7 comments · May be fixed by #3932
Open

feat: Add some way for awesome.spawn to set G_SPAWN stdio flags. #3865

aarondill opened this issue Oct 20, 2023 · 7 comments · May be fixed by #3932

Comments

@aarondill
Copy link
Contributor

aarondill commented Oct 20, 2023

The specific flags in question are:

G_SPAWN_STD{ERR,OUT,IN}_TO_DEV_NULL
G_SPAWN_CHILD_INHERITS_STD{ERR,OUT,IN}

Documentation for these flags is found here: https://docs.gtk.org/glib/flags.SpawnFlags.html

Reason for feature:

Attempting to spawn a process (using awesome.spawn directly or indirectly ((awful.spawn.*))) with it's stdio pointing to /dev/null is currently impossible (without manually spawning using lgi bindings to g_spawn_async_with_pipes, but this means snid detection can't be used).

Use case:

My personal use case is to spawn certain noisey processes (for example: browser) without outputting their results to the console.
In my configuration, awesome's stdout and stderr are redirected to a log file so I can check for errors later, but this file also includes all processes spawned by awesome.spawn.
I tried using the return_std{in,out,err} parameters to avoid this issue, but ended up running out of FD and being unable to spawn processes. (super frustrating to debug...)
Using the return_std{in,out,err} parameters, you could return parameters and close them, but most applications will die on SIGPIPE (from closing stdout/stderr) so this is not feasible. Additionally, if return_stdin is false, stdin is connected to /dev/null anyways, so this becomes entirely pointless.

Current implementation:

The options given for awesome.spawn are booleans that determine whether file descriptors should be returned.
If false or nil is passed for stdout/stderr then the program will inherit the corresponding stream from awesome, and if true is passed, a new fd is created and returned for use in the program (presumably using LGI).
This makes it impossible to simply discard the output (redirect to /dev/null)

note: stdin will be redirected from /dev/null by default (see: https://docs.gtk.org/glib/flags.SpawnFlags.html#child_inherits_stdin).
This instead makes it impossible to inherit stdin from awesome (unlikely situation, but perhaps a program needs console access?)

Desired result:

Ideally, there would be a way to indicate to awesome.spawn that stdout/stderr should not be returned, but rather redirected to /dev/null.

Possible Implementations:

  • Another parameter :(
    • Pros: backwards compatability
    • Cons: Another parameter. Given that awesome.spawn takes 7 params right now, this seems bad.
  • Expand the return_std{in,out,err} params to allow non-boolean parameters (See below for examples).
    • Pros: Potential expansion?
    • Cons: Multiple types must now be checked and supported.
  • Finally, we could drop the 7 parameters and replace it with a table?
    • Pros:
      • Much more readable than the current implementation
      • Allows future expansion of parameters if desired
    • Cons:
      • the previous implementation would need to be maintained for backwards compatibility, but that could be done in lua (?)
      • the C implementation to get values from this table could be difficult? Sadly, I don't know enough C (or the lua VM) to know, but it seems more complex from the C side.

Examples for second implementation (expand return_std{in,out,err})

The second implementation could be an integer with special values. For example:

  • -1 means inherit stream (only really useful for stdin, see note above)
  • 0 means return fd for stream (true does too for backward compatibility)
  • 1 means redirect to '/dev/null'
  • false would mean keep the GLIB default values (for backward compatibility)

Or, it could be a string. For example:

  • true means return fd for stream
  • false means keep the GLIB default values (for backward compatibility)
  • "/dev/null" means redirect to '/dev/null'
  • "inherit" means inherit stream (only really useful for stdin, see note above)
  • other values could potentially be used as a filename to redirect the output stream to?
    • If this was chosen, then "inherit" might need to be changed?
    • not sure if this is possible using g_spawn_async_with_pipes.
@aarondill aarondill changed the title Add some way for awesome.spawn to set G_SPAWN stdio flags. feat: Add some way for awesome.spawn to set G_SPAWN stdio flags. Oct 20, 2023
@aarondill
Copy link
Contributor Author

aarondill commented Oct 20, 2023

For some additional concrete real-world context, here's the chronology of my attempt to reduce the number of open FDs (from 2048!) while still keeping my log readable (chromium really hates my computer and complains about it every time a pointer event occurs.)

aarondill added a commit to aarondill/awesome that referenced this issue Oct 27, 2023
Close stdout and stderr
In this specific case, this is fine because chromium-based browsers open cat processes for their stdio
These processes can die without the browser being killed by sigpipe.
See awesomeWM/awesome#3865 for the (eventual) better way to do this.
@psychon
Copy link
Member

psychon commented Oct 31, 2023

Half un-related (this is a hack and not a proper solution): Replace awesome.spawn("browser") with awesome.spawn_with_shell("browser > /dev/null") (stderr can be redirected in the same way if needed). For bonus points, use exec browser to get rid of the shell after it did its job.

@aarondill
Copy link
Contributor Author

aarondill commented Oct 31, 2023

awesome.spawn_with_shell isn't a function, you're thinking awful.spawn.with_shell, but awesome.spawn({"sh", "-c", "exec browser &>/dev/null"}) would work the way you specify, however this remove the error detection (ie, no longer returns a string and now requires an exit_callback to check for), and also opens the door for potential shell injections (which are avoided with awesome.spawn normally)

@Elv13
Copy link
Member

Elv13 commented Nov 19, 2023

I don't have that much time right now. Do you think you can do a pull request? You have all the context.

A better alternative would be to implement the nonsense required for systemd cgroup based replacement for started notification to work. It's part of the same code (and both could be done at the same time). For that, some code in between the fork and the exec need to be added to "wait" for the process to be moved to the group before calling exec on the spanned process. glib has most of the APIs to do it. With that implemented, along with the startup id, you get a process group and you are able to keep tracking which awful.spawn was the source of clients even if there are multiple ones.

@aarondill
Copy link
Contributor Author

@Elv13 I'm sorry, I don't think I understand how using systems cgroups would effectively replace the current startup notifications or how they would fix the original issue by allowing the user to dictate where the process' sys{in,out,err} is connected?

Also, I'm afraid however we end up deciding to implement this, I likely won't be much help, as I'm only mildly familiar with C and glib. I'm actively learning to read C, but writing it is a whole other story, and in terms of glib, I've gotten frustratingly familiar with it, but I've only used it though lgi in lua (almost certainly a much nicer experience than using it in C, and that's saying a lot because its quirks are annoying enough in lua)

@psychon

This comment was marked as off-topic.

aarondill added a commit to aarondill/awesome-1 that referenced this issue Jun 30, 2024
Fixes: awesomeWM#3865
Currently works by allowing the exact strings "DEV_NULL" or "INHERIT" to
be passed to return_std*.

Documentation / error messages need to be fixed.
@aarondill
Copy link
Contributor Author

@Elv13 I've created a PR in #3932 to add this functionality. I'd appreciate a review when you have time.

aarondill added a commit to aarondill/awesome-1 that referenced this issue Jun 30, 2024
Fixes: awesomeWM#3865
Currently works by allowing the exact strings "DEV_NULL" or "INHERIT" to
be passed to return_std*.

Documentation / error messages need to be fixed.

Signed-off-by: aarondill <[email protected]>
aarondill added a commit to aarondill/awesome-1 that referenced this issue Jun 30, 2024
Fixes: awesomeWM#3865
Currently works by allowing the exact strings "DEV_NULL" or "INHERIT" to
be passed to return_std*.

Documentation / error messages need to be fixed.

Signed-off-by: aarondill <[email protected]>
aarondill added a commit to aarondill/awesome-1 that referenced this issue Jul 17, 2024
Fixes: awesomeWM#3865
Currently works by allowing the exact strings "DEV_NULL" or "INHERIT" to
be passed to return_std*.

Signed-off-by: aarondill <[email protected]>
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 a pull request may close this issue.

3 participants