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

double forking for process spawning #6828

Open
pg83 opened this issue Feb 10, 2022 · 16 comments
Open

double forking for process spawning #6828

pg83 opened this issue Feb 10, 2022 · 16 comments
Labels
enhancement New feature or incremental improvement

Comments

@pg83
Copy link

pg83 commented Feb 10, 2022

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 pg83 added the enhancement New feature or incremental improvement label Feb 10, 2022
@pg83
Copy link
Author

pg83 commented Feb 11, 2022

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

@ghost
Copy link

ghost commented Feb 19, 2022

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
Copy link
Author

pg83 commented Feb 19, 2022

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
Copy link
Author

pg83 commented Feb 19, 2022

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
Copy link
Member

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

@pg83
Copy link
Author

pg83 commented Feb 22, 2022

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
Copy link
Author

pg83 commented Feb 22, 2022

posix_spawn

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

@bqv
Copy link

bqv commented Feb 22, 2022

+1, im curious for this behaviour.

@pg83
Copy link
Author

pg83 commented Mar 10, 2022

Any chance to progress here?

@bqv
Copy link

bqv commented Mar 10, 2022

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

@pg83
Copy link
Author

pg83 commented Mar 10, 2022

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

Very interesting, i'll try it.

@pg83
Copy link
Author

pg83 commented Mar 13, 2022

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
Copy link
Author

pg83 commented Mar 25, 2022

@emersion any comments, please?

@emersion
Copy link
Member

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

@mstoeckl
Copy link
Contributor

mstoeckl commented Nov 26, 2022

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 qbs/qbs@557775b .) 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or incremental improvement
Development

No branches or pull requests

4 participants