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

expose xwayland.Server's pid #207

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tych0
Copy link

@tych0 tych0 commented Nov 27, 2024

wlroots wants to fork() and waitpid() on the result:

https://gitlab.freedesktop.org/wlroots/wlroots/-/commit/871646d22522141c45db2c0bfa1528d595bb69df

(this code almost does the right thing, but only does SIG_BLOCK in the first fork, not the parent...)

applications using wlroots which have SIGCHLD handlers installed may interfere with this, so expose the server pid so they can use WNOWAIT trickery to avoid reaping this pid, to allow wlroots to do so.

Maybe this is the wrong patch to fix the described issue: maybe pywayland should block SIGCHLD (via signal.pthread_sigmask(), since applications may be multithreaded, especially if they use asyncio) and then do wlr_xwayland_server_create(), then unblock it for users? But either way, seems like people might want to see the pid here.

wlroots wants to fork() and waitpid() on the result:

https://gitlab.freedesktop.org/wlroots/wlroots/-/commit/871646d22522141c45db2c0bfa1528d595bb69df

(this code *almost* does the right thing, but only does SIG_BLOCK in the
first fork, not the parent...)

applications using wlroots which have SIGCHLD handlers installed may
interfere with this, so expose the server pid so they can use WNOWAIT
trickery to avoid reaping this pid, to allow wlroots to do so.

Maybe this is the wrong patch to fix the described issue: maybe pywayland
should block SIGCHLD (via signal.pthread_sigmask(), since applications may
be multithreaded, especially if they use asyncio) and then do
wlr_xwayland_server_create(), then unblock it for users? But either way,
seems like people might want to see the pid here.

Signed-off-by: Tycho Andersen <[email protected]>
@Consolatis
Copy link

Consolatis commented Nov 27, 2024

That is what labwc is doing as well, it needs to clean up processes it spawns itself (+ get notified when a primary client terminates) and thus has a global SIGCHLD handler. That indeed breaks lazy initialization of xwayland and therefore labwc uses WNOWAIT to peek at the pid and compares it with the xwayland pid before reaping.

@tych0
Copy link
Author

tych0 commented Nov 27, 2024

Yeah, and actually doing the ptrace_setsigmask() trick won't work, because then the SICHLD will be delivered somewhere else. It really feels like wlroots shouldn't fork here in favor of posix_spawn() or something.

@tych0
Copy link
Author

tych0 commented Nov 27, 2024

Oh... it turns out they already do a pipe to notify, and the waitpid() that causes the error is really just about zombie prevention, I think. I sent them a patch: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4926

nschloe pushed a commit to live-clones/wlroots that referenced this pull request Dec 6, 2024
SIGCHLD here isn't fatal: we have other means of notifying that things were
successful or failure, and it causes many compositors to have to do a bunch
of extra work:

qtile/qtile#5101
flacjacket/pywlroots#207 (comment)
djpohly/dwl#212

Signed-off-by: Tycho Andersen <[email protected]>
@heuer heuer mentioned this pull request Dec 10, 2024
6 tasks
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.

2 participants