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.

Sign up for GitHub

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

Jump to bottom

Handle BrokenPipeError gracefully (#4170) #5907

Merged
merged 3 commits into from
Jan 21, 2019
Merged

Handle BrokenPipeError gracefully (#4170) #5907

merged 3 commits into from
Jan 21, 2019

Conversation

cjerdonek
Copy link
Member

This shows an alternative way of addressing #4170.

Here is sample output:

$ pip list | head
Package           Version    Location                                                                                  
----------------- ---------- ----------
CacheControl      0.12.5     
...
idna              2.7        
ERROR: Pipe to stdout was broken
Exception ignored in: <_io.TextIOWrapper name='<stdout>' mode='w' encoding='UTF-8'>
BrokenPipeError: [Errno 32] Broken pipe

And with more verbose output:

$ pip -v list | head
Package           Version    Location                                                                                   Installer
----------------- ---------- ------------
CacheControl      0.12.5
...
idna              2.7
ERROR: Pipe to stdout was broken
Traceback (most recent call last):
  File "/.../logging/__init__.py", line 996, in emit
    self.flush()
  File "/.../logging/__init__.py", line 976, in flush
    self.stream.flush()
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/.../pip/_internal/cli/base_command.py", line 144, in main
    status = self.run(options, args)
  File "/.../pip/_internal/commands/list.py", line 145, in run
    self.output_package_listing(packages, options)
  File "/.../pip/_internal/commands/list.py", line 211, in output_package_listing
    self.output_package_listing_columns(data, header)
  File "/.../pip/_internal/commands/list.py", line 234, in output_package_listing_columns
    logger.info(val)
  File "/.../logging/__init__.py", line 1306, in info
    self._log(INFO, msg, args, **kwargs)
  File "/.../logging/__init__.py", line 1442, in _log
    self.handle(record)
  File "/.../logging/__init__.py", line 1452, in handle
    self.callHandlers(record)
  File "/.../logging/__init__.py", line 1514, in callHandlers
    hdlr.handle(record)
  File "/.../logging/__init__.py", line 863, in handle
    self.emit(record)
  File "/.../logging/__init__.py", line 998, in emit
    self.handleError(record)
  File "/.../pip/_internal/utils/logging.py", line 133, in handleError
    raise BrokenLoggingStdoutError()
pip._internal.utils.logging.BrokenLoggingStdoutError
Exception ignored in: <_io.TextIOWrapper name='<stdout>' mode='w' encoding='UTF-8'>
BrokenPipeError: [Errno 32] Broken pipe

@cjerdonek cjerdonek mentioned this pull request Oct 21, 2018
Closed
@cjerdonek cjerdonek force-pushed the broken-pipe-error branch 19 times, most recently from 79185cc to 764f9a0 Compare October 23, 2018 09:42
pradyunsg
pradyunsg reviewed Oct 23, 2018
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I like the approach here.

src/pip/_internal/cli/base_command.py Show resolved Hide resolved
@cjerdonek cjerdonek added C: logging Information Logging T: bugfix C: cli Command line interface related things (optparse, option grouping etc) labels Oct 24, 2018
@pradyunsg
Copy link
Member

@cjerdonek Is this still a WIP?

@cjerdonek
Copy link
Member Author

@pradyunsg I was hoping to add some higher-level tests, like testing the exception handling in base_command.py. Currently just the logging aspect is being tested. But otherwise I'm okay with things.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jan 10, 2019
@cjerdonek cjerdonek changed the title [WIP] Handle BrokenPipeError gracefully (#4170) Handle BrokenPipeError gracefully (#4170) Jan 15, 2019
@cjerdonek
Copy link
Member Author

This is now ready to be reviewed.

@cjerdonek
Copy link
Member Author

Hi @pradyunsg, I just wanted to flag this PR for you. Is it okay for me to add this to the milestone for the upcoming release? The non-test code portion had been implemented and done for a while, and it now has very good test coverage. This includes a thorough end-to-end test (using subprocess) passing on all platforms. It is a bug fix for a quite visible problem when it happens

@cjerdonek cjerdonek force-pushed the broken-pipe-error branch 3 times, most recently from e7bd2b9 to f7f463b Compare January 19, 2019 21:37
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jan 20, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jan 20, 2019
@pradyunsg pradyunsg added this to the 19.0 milestone Jan 21, 2019
@pradyunsg
Copy link
Member

Hi @pradyunsg, I just wanted to flag this PR for you.

Whoops! I haven't been checking my GitHub notifications properly.

Is it okay for me to add this to the milestone for the upcoming release?

Yes, it is. Let's include this in the next release.

As an aside, the way we've mostly flagged things for being added to a release since I've been around, is to simply add them to the release milestone and leave a comment before/after adding it. If the feature is not a blocker, the RM will remove it when the release comes. This is the sort of "institutional knowledge" that I think we should start documenting in out

GitHub mentions (thanks notifications) usually get buried.

@pradyunsg pradyunsg merged commit fc9bb60 into pypa:master Jan 21, 2019
@pradyunsg
Copy link
Member

pradyunsg commented Jan 21, 2019 •
edited
Loading

@cjerdonek this is a super polished PR, with nice comments in the right places and understandable code. Nice work! 🎉

@cjerdonek
Copy link
Member Author

Thanks a lot for merging and for the nice comments, @pradyunsg. This was one of my more involved PR's, so I appreciate it!

@cjerdonek cjerdonek deleted the broken-pipe-error branch January 21, 2019 20:23
This was referenced Jan 21, 2019
Merged
Closed
Closed
Closed
Merged
Closed
@lock
Copy link

lock bot commented May 30, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: cli Command line interface related things (optparse, option grouping etc) C: logging Information Logging
Projects
None yet
Milestone
19.0
Development

Successfully merging this pull request may close these issues.
4 participants
@cjerdonek @pradyunsg @BrownTruck @pypa-bot