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

Reap zombie processes left behind by openURL #1928

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

Conversation

pfirsich
Copy link
Contributor

This was attempted to be fixed in f7d226d but sadly broke os.execute (#1047), so it was later removed in a4a62f3.
This is a very similar fix, but we make sure to only waitpid for processes that we are supposed to reap.

Without this fix an example program like this would leave behind zombie processes:

function love.load()
    love.system.openURL("https://theshoemaker.de")
    love.system.openURL("https://google.de")
end
$ ps --ppid (pidof love)
    PID TTY          TIME CMD
 133235 pts/2    00:00:00 xdg-open <defunct>
 133236 pts/2    00:00:00 xdg-open <defunct>

With the fix, those zombie processes are gone.
I also made sure that this doesn't also break os.execute. This example program:

function love.load()
    love.system.openURL("https://theshoemaker.de")
    love.system.openURL("https://google.de")
    print(os.execute("false"))
    print(os.execute("true"))
end

prints

256
0

I am not sure if my comment in the code is too verbose, but considering it took me a while to figure out what went on there in the past, I thought it would be better to write too much than too little. Let me know if anything needs changing.

Also I am not sure if that fix is even worth it. It's not very clean to leave them behind and they might be annoying, but the typical löve program does not open enough URLs for this to become a real problem.

This was attempted to be fixed in f7d226d but sadly broke os.execute
(love2d#1047), so it was later removed in a4a62f3.

This is a very similar fix, but we make sure to only waitpid for
processes that we are supposed to reap.
return spawnedPids;
}

static void sigchldHandler(int signo, siginfo_t* info, void* /*context*/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very unfamiliar with these APIs so this might be a silly question: is it possible for this callback to be called on other threads, or is it guaranteed to only be called on the main thread?

If it can be called on other threads there'll need to be some extra thread safety (maybe a mutex) when modifying spawnedPids.

sa.sa_sigaction = sigchldHandler;
sigemptyset(&sa.sa_mask);
sa.sa_flags = SA_RESTART | SA_SIGINFO;
sigaction(SIGCHLD, &sa, nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this get cleaned up / reset when the System class is destroyed? e.g. what happens when love.event.quit("restart") is called (which keeps the process running but starts new instances of love's modules)?

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