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

wip: feat: terminate jq immediately after the outgoing pipe closed #1703

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

Conversation

mumoshu
Copy link

@mumoshu mumoshu commented Aug 3, 2018

This should work. In reality, this does compile, but doesn't function when I actually try to emit SIGPIPE by doing:

cat | ./jq . | bash -c 'sleep 1; foo'
bash: foo: command not found

My understanding is that the missing command foo should immediately result in SIGPIPE on the jq command, so that
this feature terminates jq immediately.

I'd greatly appreciate it if anyone could share your insight on how I could make it actually work 🙏

Resolves #1691

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 84.48% when pulling da030b0 on mumoshu:quit-on-sigpipe into 90bc29c on stedolan:master.

{
#ifndef WIN32
signal(SIGPIPE, SIG_IGN);
fprintf(stderr, "jq: error: received SIGPIPE\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

fprintf is not async-signal-safe, so it can't be used from a signal handler.

#ifndef WIN32
signal(SIGPIPE, SIG_IGN);
fprintf(stderr, "jq: error: received SIGPIPE\n");
exit(128 + SIGPIPE);
Copy link
Contributor

Choose a reason for hiding this comment

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

exit() is not async-signal-safe, so it can't be used from a signal handler. Use _exit() instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

If and when we add an FFI this (exiting in a signal handler) will probably not be a safe thing to do.

@nicowilliams
Copy link
Contributor

See also commentary in #1285.

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