swaywm / sway

i3-compatible Wayland compositor
https://swaywm.org
MIT License
14.55k stars 1.11k forks source link

double forking for process spawning #6828

Open pg83 opened 2 years ago

pg83 commented 2 years ago

Hi.

Currently sway uses double-forking for its process spawning, for example, in exec_always.c.

1) Is there any real difference for current sway users, who(sway or init) "parents" launched processes? 2) If no, can this(or sort of) patch be applied - https://github.com/pg83/mix/blob/main/pkgs/bin/sway/supervised/exec_always.c#L48, so one can see perfect pstree listings?

pg83 commented 2 years ago

Also this patch removes syncronous calls to read/write/waitpid, increasing responsiveness of process starting(not much, of course).

pg83 commented 2 years ago

https://github.com/pg83/sway/commit/d8011541a6a04642a752eb2de719bd10022d90de#diff-71299c6ccbeae979cf588e4d15e1089d7946396ae514c03c6aa003444e29b860L53 - dit diff, for convinience

ghost commented 2 years ago

I am not a sway developer, but I think this feature is absolutely critical.

@pg83

  1. Since you are running execlp right after fork(), don't you think that a vfork() would be better?
  2. ... or maybe even posix_spawn()?
  3. I added a few comments to your pull request, which, it seems to me, have some minor problems.
pg83 commented 2 years ago

My patch is VERY preliminary. I will fix all patch problems, but, honestly, first I want to get OK/NOT OK for the idea behind patch, from sway devs :)

pg83 commented 2 years ago

posix_spawn() perfectly OK from my point of view, and even better than fork() + exec().

vfork() is a "no go" in new code, actually.

emersion commented 2 years ago

Double-forking is used to avoid all of the SIGCHLD issues. I'd rather not change it. -1 for posix_spawn.

pg83 commented 2 years ago

Can you please explain about this issues with SIGCHILD? I have run my patch for weeks now, and does not have any issues...

I'd rather not change it

Can I do anything to change your mind? May be I can implement such a behaviour under a config option? Anything I can do to get this feature in mainline sway?

I see this as non-breaking change for most users, but some of us, who wants to see completely supervised tree in pstree(for example) will benefit from this.

pg83 commented 2 years ago

posix_spawn

I have no strong opinion here, but you should know, that posix_spawn is technically superior:

bqv commented 2 years ago

+1, im curious for this behaviour.

pg83 commented 2 years ago

Any chance to progress here?

bqv commented 2 years ago

Personally i just wrapped sway in a subreaper to get the functionality. https://github.com/bqv/session

pg83 commented 2 years ago

Personally i just wrapped sway in a subreaper to get the functionality. https://github.com/bqv/session

Very interesting, i'll try it.

pg83 commented 2 years ago

Yes, this subreaper approach actually works, https://github.com/pg83/mix/blob/main/pkgs/bin/subreaper/mix.sh

But, personally, I will prefer a proper solution in sway, if this is possible at all :)

pg83 commented 2 years ago

@emersion any comments, please?

emersion commented 2 years ago

Sorry, I don't have time to look into this atm.

mstoeckl commented 1 year ago

It's quite complicated to implement properly -- the following patch is maybe 30% of the necessary effort -- but in theory it is not required, in the fork-setsid-fork-exec method of process spawning, that the first process (which calls setsid()) must exit immediately. Instead, you can keep the first process alive, and have it call waitpid(...) periodically to reap zombie processes. In fact, it's possible to create a long running server process under Sway which calls setsid(), and performs the final fork-exec steps on being asked to by the main sway process. (This technique also can reduce fork overhead -- see https://github.com/qbs/qbs/commit/557775b1ad3316ec158b9d930bf733b4fa502456 .) While the server process is alive, pstree shows all the spawned processes as children of the server/grandchildren of sway; although when sway dies, they are orphaned and no longer are grouped together under pstree; although they will have the same PGID and SID.

0001-Introduce-sway-exec-helper.patch.txt

GrabbenD commented 4 months ago

Very interesting thread, did this ever get resolved?

P.S @pg83 all of your links/patches from above are 404

pg83 commented 4 months ago

Very interesting thread, did this ever get resolved?

nope

pg83 commented 4 months ago

@pg83 all of your links/patches from above are 404

yep, I deleted my branch because the upstream was not interested.

GrabbenD commented 4 months ago

Some changes take a long time, years at times.

I think it's worth keeping working links (or a .patch as a attachment) for inspiration. Hopefully someone might pick this up in the future and submit a proper PR 🙂