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

wait isn't interruptable #858

Closed
terinjokes opened this issue Nov 5, 2020 · 5 comments
Closed

wait isn't interruptable #858

terinjokes opened this issue Nov 5, 2020 · 5 comments

Comments

@terinjokes
Copy link

I've noticed that wait isn't interruptable like it is in bash, which cased some issues with scripts relying on background sleep and trap behavior.


This is a trivial reproduction case trimmed from a project with lots of similar scripts.

action() {
	echo "from signal" | ts
}

status() {
	# do some background status report
	date
}

trap "action" USR1

echo $$ | ts
while true; do
        status
	sleep 10 &
	wait $!
done

When running in bash, sending USR1 to the process will immediately run the trap handler.

$ bash ./test.sh
Nov 05 01:07:01 7362
Thu 05 Nov 2020 01:07:01 AM PST
Thu 05 Nov 2020 01:07:11 AM PST
Nov 05 01:07:13 from signal
Thu 05 Nov 2020 01:07:13 AM PST
Nov 05 01:07:14 from signal
Thu 05 Nov 2020 01:07:14 AM PST
Nov 05 01:07:15 from signal
Thu 05 Nov 2020 01:07:15 AM PST
Thu 05 Nov 2020 01:07:25 AM PST
^C
$ kill -USR1 7362 && date
Thu 05 Nov 2020 01:07:13 AM PST
$ kill -USR1 7362 && date
Thu 05 Nov 2020 01:07:14 AM PST
$ kill -USR1 7362 && date
Thu 05 Nov 2020 01:07:15 AM PST

Running the same script in osh requires us to wait for the full 10 second sleep before the handler is executed.

$ osh ./test.sh
Nov 05 01:09:31 8770
Thu 05 Nov 2020 01:09:31 AM PST
[%1] Started PID 8774
Thu 05 Nov 2020 01:09:41 AM PST
[%2] Started PID 8857
Nov 05 01:09:51 from signal
Thu 05 Nov 2020 01:09:51 AM PST
[%3] Started PID 8933
Nov 05 01:10:01 from signal
Thu 05 Nov 2020 01:10:01 AM PST
[%4] Started PID 9001
Nov 05 01:10:11 from signal
Nov 05 01:10:11 from signal
Thu 05 Nov 2020 01:10:11 AM PST
[%5] Started PID 9094
Thu 05 Nov 2020 01:10:21 AM PST
[%6] Started PID 9178
^C
$ kill -USR1 8770 && date
Thu 05 Nov 2020 01:09:44 AM PST
$ kill -USR1 8770 && date
Thu 05 Nov 2020 01:09:55 AM PST
$ kill -USR1 8770 && date
Thu 05 Nov 2020 01:10:03 AM PST
$ kill -USR1 8770 && date
Thu 05 Nov 2020 01:10:05 AM PST
andychu pushed a commit that referenced this issue Nov 5, 2020
I guess this is about when the main loop runs signal handlers.

Unrelated: doc edits.
@andychu
Copy link
Contributor

andychu commented Nov 5, 2020

Thanks for the report! I reproduced this bug.

I think the bug is that signal handlers run in the main interpreter loop, and wait $pid runs in its own loop until the process stops.

Originally I thought this would be better fixed in the C++ translation, because signals are a bit different there, but it's possible it is an algorithmic problem. I'll look into it, thanks!

andychu pushed a commit that referenced this issue Jan 21, 2021
Addresses issue #858.

Also improve test case.  Found the same bug in dash!  bash/mksh/zsh have
special cases that allow you to interrupt the 'wait' builtin.

Also make note of bug with Ctrl-C / SIGINT, due to KeyboardInterrupt.
@andychu
Copy link
Contributor

andychu commented Jan 21, 2021

I just fixed this -- thanks for this great bug report!

It looks like bash/zsh/mksh all have a special flag to make the wait builtin interruptable, so now OSH does the same. (dash doesn't have it, so it has the same bug!)

The fix will be out with the next release.


Sort of related: I'm looking for feedback on Oil's enhanced shell tracing:

http://www.oilshell.org/preview/doc/xtrace.html

https://old.reddit.com/r/oilshell/comments/l0sshn/tracing_execution_in_oil_set_x_xtrace/

andychu pushed a commit that referenced this issue Jan 22, 2021
Also, 2 out of 3 cases work for 'interruptible wait'.  Still need to fix
the broken -1 invariant.o

Related to issue #858.
@andychu
Copy link
Contributor

andychu commented Jan 25, 2021

@andychu andychu closed this as completed Jan 25, 2021
@terinjokes
Copy link
Author

Oh wow, xtrace looks awesome for debugging.

@andychu
Copy link
Contributor

andychu commented Jan 25, 2021

Yup I just used it to debug some really poorly formatted code in #881 -- it was a big improvement. It's still early and up for discussion. More ideas on #891 !

It's out with 0.8.7 so keep it in mind when you need to debug :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants