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

shell fails to reap child processes in a timely manner #33

Closed
godmar opened this issue Jul 6, 2021 · 3 comments · Fixed by #38
Closed

shell fails to reap child processes in a timely manner #33

godmar opened this issue Jul 6, 2021 · 3 comments · Fixed by #38

Comments

@godmar
Copy link

godmar commented Jul 6, 2021

If you run

sleep 5 &
sleep 200

The first command should be reaped after 5 seconds, not after 200.

Similarly, if you run

sleep 1 &

this job isn't reaped until the user hits Enter again.

@mitnk
Copy link
Owner

mitnk commented Jul 6, 2021

Good catch! Just tried the same in bash, no such issue. (my first thought was bash do the same thing :)

Seems I need to add a signal handler for SIGCHLD.

Thanks for trying cicada.

@godmar
Copy link
Author

godmar commented Jul 6, 2021

You should call waitpid(-1,...) and then handle whatever job the process belongs to while you wait for a foreground job.

In addition, you do need a SIGCHLD handler, but this is tricky in Rust since the runtime is not async-signal -safe. Since the shell is either waiting for user input or for a foreground job you could unblock SIGCHLD right before reading user input, then block it right after.

@mitnk
Copy link
Owner

mitnk commented Jul 10, 2021

I tried a way to address this issue in #38, But feel it's not that ideal (but cannot find a better way yet): I have to sync some information between the signal hander and the main thread. One issue is if the same PID is used by another new process and it also got reaped by hander. Incorrect status may be fetched by main thread.

@godmar would you like to have a look and provide me any suggestions? Thanks a lot!

@mitnk mitnk closed this as completed in #38 Jul 12, 2021
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 a pull request may close this issue.

2 participants