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

wl-copy does not close stderr when forking #212

Open
sfan5 opened this issue Feb 10, 2024 · 5 comments
Open

wl-copy does not close stderr when forking #212

sfan5 opened this issue Feb 10, 2024 · 5 comments

Comments

@sfan5
Copy link

sfan5 commented Feb 10, 2024

https://github.com/bugaevc/wl-clipboard/blob/master/src/wl-copy.c#L65

This trips up applications that try to capture output from child processes (because stderr remains open)

@sfan5
Copy link
Author

sfan5 commented Feb 11, 2024

Seeing as this has been brought up before in #110 and #154 let me elaborate:

A reasonably common case for mpv Lua scripts is to copy something to the clipboard.
This is achieved using something like this:

local function copy_to_clipboard(text)
	if os.getenv("WAYLAND_DISPLAY") then
		local r = mp.command_native({
			name = "subprocess",
			args = {"wl-copy", text},
			playback_only = false,
		})
		if r.status == 0 then
			return true
		end
	else
		-- omitted for brevity
	end
	msg.error("Error writing clipboard")
end

The only problem with this is that is hangs, until something else is selected because at that point wl-copy exits.
Now mpv does not try to observe the child process specifically, but it internally captures the stdout and stderr output so it blocks until the child process has exited regardless.

For me what speaks against keeping stderr open:

  • too late to properly signal an error anyway (exit code)
  • xclip doesn't cause the same issue (and I'm pretty sure it has to fork too)
  • likely to trigger exactly these edge cases where parent applications don't expect a child to fork and keep one of its fds

Now I understand you don't want to swallow the error so how about logging to syslog instead?

@sfan5
Copy link
Author

sfan5 commented Mar 16, 2024

@bugaevc thoughts?

@bugaevc
Copy link
Owner

bugaevc commented Mar 17, 2024

Hi,

The only problem with this is that is hangs, until something else is selected because at that point wl-copy exits.
Now mpv does not try to observe the child process specifically, but it internally captures the stdout and stderr output so it blocks until the child process has exited regardless.

well, this sounds like something to be fixed in mpv, no?

xclip doesn't cause the same issue (and I'm pretty sure it has to fork too)

It does fork, but are you sure mpv doesn't have the same issue with xclip? It certainly doesn't look like xclip closes its stderr (or stdout), neither from skimming their source code, nor in practice:

$ echo hi | strace -f --trace-fds=0,1,2 xclip -i &
[1] 216777
newfstatat(0, "", {st_mode=S_IFIFO|0600, st_size=0, ...}, AT_EMPTY_PATH) = 0
read(0, "hi\n", 4096)                   = 3
read(0, "", 4096)                       = 0
strace: Process 216785 attached
[pid 216784] +++ exited with 0 +++
$ echo "This is sent to xclip's stderr" > /proc/216785/fd/2
This is sent to xclip's stderr
$ readlink /proc/216785/fd/2 /proc/self/fd/2
/dev/pts/3
/dev/pts/3

Now I understand you don't want to swallow the error so how about logging to syslog instead?

That would make sense, yes. Do you know if there's a way to do this nicely? It's possible to send messages to the log using explicit syslog(3) calls, but that won't handle text that just gets printed to stderr by most of wl-clipboard's code, or by library functions. There's logger(1) and systemd-cat(1), but that'd be an overkill.

@sfan5
Copy link
Author

sfan5 commented Mar 18, 2024

I re-did my testing and you're right. xclip behaves exactly the same and results in the same issue with mpv.
I did not realize that my original code using xclip looked totally different (popen+write+close), which of course behaved totally differently.

well, this sounds like something to be fixed in mpv, no?

True. This unfortunately requires some annoying workarounds since you can't poll for the death of a process (not portably at least).

That would make sense, yes. Do you know if there's a way to do this nicely?

wl-copy's code could be refactored to use a logging mechanism that can be redirected. I believe external libraries printing directly to stderr is considered bad practice so hopefully that's not a thing that actually happens.
Failing that a more lightweight way would be to replace stderr with a pipe and have a thread feed the output to syslog(3).

If you don't want to pursue this request I can understand, however looking at the issues other people have opened relating to this behavior I think it would be worthwhile.

@bugaevc
Copy link
Owner

bugaevc commented Mar 20, 2024

This unfortunately requires some annoying workarounds since you can't poll for the death of a process (not portably at least).

FWIW, you can combine poll() with signals portably like this:

volatile sig_atomic_t got_sigchld= 0;

static void sigchld_handler(int signum) {
    assert(signum == SIGCHLD);
    got_sigchld = 1;
}

// Block SIGCHLD, remembering the previous mask.
sigset_t sigmask;
sigprocmask(SIG_SETMASK, NULL, &sigmask);
sigaddset(&sigmask, SIGCHLD);
sigprocmask(SIG_SETMASK, &sigmask, &sigmask);

while (!got_sigchld) {
    int rc = ppoll(pollfds, nfds, &timeout, &sigmask);
    if (rc == -1 && errno == EINTR) continue;
    // handle pollfds...
}
// The child has exited, wait for it (or something).
wait(...);

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

No branches or pull requests

2 participants