Handle SIGPIPE in click.echo(). #626

ob · 2016-08-03T17:58:41Z

When the output of a click program is sent to another program via a
pipe, and the pipe is closed prematurely, click.echo() was printing an
exception backtrace instead of silently exiting like most command line
utilities.

Fixes: #625

When the output of a click program is sent to another program via a pipe, and the pipe is closed prematurely, click.echo() was printing an exception backtrace instead of silently exiting like most command line utilities.

ob · 2016-09-12T16:07:11Z

Any interest in this fix?

click/utils.py

@@ -1,5 +1,6 @@
 import os
 import sys
+import errno


ThiefMaster · 2016-09-12T16:10:08Z

LGTM, but I'd rather have someone who did some actual click development look over it. General suggestion: Squash those three commits into a single one. The last two just fix issues in the initial one after all, no need for that to be part of the project's permanent history ;)

ob · 2016-09-12T16:28:09Z

I thought commit squashing was up to whoever merged the branch...

https://github.com/blog/2141-squash-your-commits

I can squash the commits if that is preferable.

ThiefMaster · 2016-09-12T16:28:57Z

True, we can do it too. Forgot about that ;)

untitaker · 2016-09-12T16:35:50Z

This doesn't silently exit, it swallows the exception and lets the application continue. IMO SIGPIPE should not be catched on this level but much further up (somewhere near subcommand dispatch) and then the traceback suppressed (but still exiting).

ob · 2016-09-12T16:40:58Z

Oops... I'm missing a sys.exit() there. I couldn't find a place to add a test so I ended up screwing up the fix ;-/

I'll take a look at subcommand dispatch and see if I can figure out where you mean.

untitaker · 2016-09-12T16:51:52Z

I think here: https://github.com/pallets/click/blob/master/click/core.py#L700

@mitsuhiko Do you agree this is the proper way to handle SIGPIPE, or should the signal module be used?

ob · 2016-09-12T16:55:42Z

Pushed something better...

ob · 2016-09-12T16:56:34Z

Ah, and now I read your comment and I see your suggestion is probably better...

ob · 2016-09-12T17:00:35Z

Something like this?

https://gist.github.com/ob/2fb860d5aea6a75c2ef3863c7b4b255d

untitaker · 2016-09-12T17:03:00Z

Yeah, but is sys.exit(EPIPE) the right thing to do?

ob · 2016-09-12T17:04:53Z

I was just googling that... it seems exiting -1 might be better... I tried a few system utilities and they seem to exit 0. I was about to go read POSIX :)

untitaker · 2016-09-12T17:06:45Z

I think we're supposed to raise Abort.

ob · 2016-09-12T17:08:38Z

The problem with raising Abort is that it prints "Aborted!" to stderr. This is undesirable for things like piping the output to head for example.

untitaker · 2016-09-12T17:10:09Z

Fair enough, then sys.exit(1) is probably the way to go.

ob · 2016-09-12T17:21:55Z

Okay, I pushed that. BTW, I'm so counting on you squashing all this mess ;)

ob · 2016-09-16T18:10:13Z

Is this acceptable now?

untitaker · 2016-09-16T20:20:41Z

I'll probably merge in a few days. It looks fine to me, but code review at
conferences is a bad idea.

On Fri, Sep 16, 2016 at 11:10:14AM -0700, Oscar Bonilla wrote:

Is this acceptable now?

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#626 (comment)

sirosen · 2016-09-17T17:39:27Z

Although I agree that click.echo should capture EPIPE by default, could we add an option somewhere to have it raise Abort or reraise the IOError instead?

I have a bit of code that's doing

try:
    click.echo(message, nl=newline, err=write_to_stderr)
except IOError as err:
    if err.errno is errno.EPIPE:
        pass
    else:
        raise

So that I can maintain my promises about exit codes.

This would be, to me, an acceptable alternative:

try:
    click.echo(message, nl=newline, err=write_to_stderr, handle_epipe=False)
except IOError:
    # a reraised IOError from inside of click.echo! Must be EPIPE
    pass

What I object to is being asked to do this:

try:
    click.echo(message, nl=newline, err=write_to_stderr)
except SystemExit:
    # I guess this was an EPIPE?
    pass

As a caller into click.echo, having it decide to raise SystemExit seems wrong.
Consider:

set -o pipefail
mycommand | head -n 20 | grep 'importantstring'

If supporting a pipeline like that is a priority for mycommand, it shouldn't be deprived of the features of click.echo.

untitaker · 2016-09-19T17:10:55Z

Your code doesn't have to change since SystemExit is raised somewhere completely different than from within echo. The PR title is now wrong.

On 17 September 2016 19:39:29 CEST, Stephen Rosen notifications@github.com wrote:

Although I agree that click.echo should capture EPIPE by default,
could we add an option somewhere to have it raise Abort or reraise
the IOError instead?

I have a bit of code that's doing
try:
   click.echo(message, nl=newline, err=write_to_stderr)
except IOError as err:
   if err.errno is errno.EPIPE:
       pass
   else:
       raise
So that I can maintain my promises about exit codes.

This would be, to me, an acceptable alternative:
try:
click.echo(message, nl=newline, err=write_to_stderr,
handle_epipe=False)
except IOError:
   # a reraised IOError from inside of click.echo! Must be EPIPE
   pass
What I object to is being asked to do this:
try:
   click.echo(message, nl=newline, err=write_to_stderr)
except SystemExit:
   # I guess this was an EPIPE?
   pass
As a caller into click.echo, having it decide to raise SystemExit
seems wrong.
Consider:
set -o pipefail
mycommand | head -n 20 | grep 'importantstring'
If supporting a pipeline like that is a priority for mycommand, it
shouldn't be deprived of the features of click.echo.

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#626 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

When the output of a click program is sent to another program via a pipe, and the pipe is closed prematurely, click.echo() was printing an exception backtrace instead of silently exiting like most command line utilities. Fix #626 Fix #625

untitaker · 2016-09-19T17:23:26Z

Thanks! Please add yourself to AUTHORS if you want!

sirosen · 2016-09-19T17:25:28Z

@untitaker Thanks for pointing out that this handling is, in fact, safe for callers of click.echo.

Please correct me if I'm wrong, but as I understand it this catches an EPIPE that makes its way up to click.Command.main(), so it handles click.echo(), print(), and anything else that may encounter a broken pipe inside of the running command.
If you want re-title the PR, I'd suggest "Handle EPIPE by exiting silently with status 1".

Anyway, as always, thanks for this excellent CLI toolkit.

untitaker · 2016-09-19T17:34:14Z

Sorry, yes, I forgot to rename the PR and a same-named commit is now in master :(

On Mon, Sep 19, 2016 at 10:25:29AM -0700, Stephen Rosen wrote:

@untitaker Thanks for pointing out that this handling is, in fact, safe for callers of click.echo.

Please correct me if I'm wrong, but as I understand it this catches an EPIPE that makes its way up to click.Command.main(), so it handles click.echo(), print(), and anything else that may encounter a broken pipe inside of the running command.
If you want re-title the PR, I'd suggest "Handle EPIPE by exiting silently with status 1".

Anyway, as always, thanks for this excellent CLI toolkit.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#626 (comment)

mitsuhiko · 2017-09-20T19:04:54Z

@untitaker i just discovered this merge and I'm not particularly happy with this because this captures all sigpipes, so even on sockets. Also not sure if click should handle this by default like this. At least maybe make it optional?

ThiefMaster · 2017-09-20T19:06:08Z

Any way of checking if SIGPIPE was caused by stdin/stdout/stderr? I don't think getting a traceback in these cases is something you'd want

mitsuhiko · 2017-09-20T19:07:43Z

No, not really. You can only catch down individual writes (like in echo) and rethrow it.

untitaker · 2017-09-20T19:12:54Z

In which situation would you have a SIGPIPE from e.g. a socket? I'd vote for optional since I think this is still reasonable default behavior (assuming more fine-grained handling isn't possible).

…

On Wed, Sep 20, 2017 at 12:04:58PM -0700, Armin Ronacher wrote: @untitaker i just discovered this merge and I'm not particularly happy with this because this captures *all* sigpipes, so even on sockets. Also not sure if click should handle this by default like this. At least maybe make it optional? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #626 (comment)

mitsuhiko · 2017-09-20T19:14:10Z

You get a sigpipe when the socket is not connected and you write to it.

untitaker · 2017-09-20T19:24:15Z

Ah damn. Feel free to revert then. In general I would say that Click could use some sort of exception handling like it's known from webframeworks, but that's a different topic.

…

On Wed, Sep 20, 2017 at 07:14:15PM +0000, Armin Ronacher wrote: You get a sigpipe when the socket is not connected and you write to it. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #626 (comment)

mitsuhiko · 2017-09-21T07:14:12Z

@untitaker what about us wrapping both text and binary streams and augment the exception with from_stdio = True? Then we can capture that only by default.

untitaker · 2017-09-21T08:33:29Z

I thought about that too but that's monkeypatching. Having it in click.echo would be less surprising I think.

…

On 21 September 2017 09:14:17 GMT+02:00, Armin Ronacher ***@***.***> wrote: @untitaker what about us wrapping both text and binary streams and augment the exception with `from_stdio = True`? Then we can capture that only by default. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #626 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

mitsuhiko · 2017-09-21T08:35:13Z

We could also raise a subclass of IOError.

untitaker · 2017-09-23T05:41:45Z

Whatever you think is best, however I dont think people will apprechiate us patching stdio even if it behaves almost the same.

…

On 21 September 2017 10:35:18 GMT+02:00, Armin Ronacher ***@***.***> wrote: We could also raise a subclass of `IOError`. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #626 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

mitsuhiko · 2017-09-23T09:37:14Z

@untitaker the proposal is not to patch stdio but to change our wrappers. We already use wrappers for the unicode support and there are already accessors for getting binary streams. We can trivially further customize those streams.

untitaker · 2017-09-23T10:06:35Z

Sure, I'm totally fine with echo rewrapping this exception.

…

On 23 September 2017 11:37:20 GMT+02:00, Armin Ronacher ***@***.***> wrote: @untitaker the proposal is not to patch stdio but to change our wrappers. We already use wrappers for the unicode support and there are already accessors for getting binary streams. We can trivially further customize those streams. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #626 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

ob added 3 commits August 3, 2016 10:53

Handle SIGPIPE in click.echo().

a91aa1f

When the output of a click program is sent to another program via a pipe, and the pipe is closed prematurely, click.echo() was printing an exception backtrace instead of silently exiting like most command line utilities.

Add errno to the list of allowed imports.

6a601af

Python 3.

ca06783

ThiefMaster reviewed Sep 12, 2016
View reviewed changes

click/utils.py

@@ -1,5 +1,6 @@

import os

import sys

import errno

This comment was marked as off-topic.

Sign in to view

Keep imports sorted.

2cdd021

Revert my previous fix and lift it to core.py.

bda5583

Fix the fix with a better fix.

0d3fdfe

untitaker closed this Sep 19, 2016

wasade mentioned this pull request Jan 17, 2017

BrokenPipeError when piping over stdin #712

Closed

mahmoud mentioned this pull request May 9, 2018

CLI robustness mahmoud/glom#8

Open

xiaket mentioned this pull request Jul 17, 2018

BrokenPipeError when piping pipenv graph output to some commands pypa/pipenv#2594

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020

Handle SIGPIPE in click.echo(). #626

Handle SIGPIPE in click.echo(). #626

Conversation

ob commented Aug 3, 2016

ob commented Sep 12, 2016

This comment was marked as off-topic.

ThiefMaster commented Sep 12, 2016

ob commented Sep 12, 2016

ThiefMaster commented Sep 12, 2016

untitaker commented Sep 12, 2016 • edited Loading

ob commented Sep 12, 2016

untitaker commented Sep 12, 2016

ob commented Sep 12, 2016

ob commented Sep 12, 2016

ob commented Sep 12, 2016

untitaker commented Sep 12, 2016

ob commented Sep 12, 2016

untitaker commented Sep 12, 2016

ob commented Sep 12, 2016 • edited Loading

untitaker commented Sep 12, 2016

ob commented Sep 12, 2016

ob commented Sep 16, 2016

untitaker commented Sep 16, 2016

sirosen commented Sep 17, 2016

untitaker commented Sep 19, 2016

untitaker commented Sep 19, 2016

sirosen commented Sep 19, 2016

untitaker commented Sep 19, 2016

mitsuhiko commented Sep 20, 2017

ThiefMaster commented Sep 20, 2017

mitsuhiko commented Sep 20, 2017

untitaker commented Sep 20, 2017 via email

mitsuhiko commented Sep 20, 2017

untitaker commented Sep 20, 2017 via email

mitsuhiko commented Sep 21, 2017

untitaker commented Sep 21, 2017 via email

mitsuhiko commented Sep 21, 2017

untitaker commented Sep 23, 2017 via email

mitsuhiko commented Sep 23, 2017

untitaker commented Sep 23, 2017 via email

untitaker commented Sep 12, 2016 •

edited

Loading

ob commented Sep 12, 2016 •

edited

Loading