Skip to content

bpo-39828: Fix json.tool to catch BrokenPipeError. #18779

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

Merged
merged 8 commits into from
Mar 10, 2020
Merged

bpo-39828: Fix json.tool to catch BrokenPipeError. #18779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f1fc4ab
bpo-39828: Fix json.tool to ignore BrokenPipeError.
corona10 Mar 4, 2020
dfa361d
bpo-39828: Update
corona10 Mar 5, 2020
871eabf
bpo-39828: Update
corona10 Mar 5, 2020
337fdd2
bpo-39828: Apply Victor's code review
corona10 Mar 7, 2020
d02c089
bpo-39828: Update NEWS.d
corona10 Mar 7, 2020
f67630c
bpo-39828: Add unittest
corona10 Mar 8, 2020
e6931d8
bpo-39828: Apply Victor's review
corona10 Mar 10, 2020
e9c7eaa
bpo-39828: Ignore test for Windows
corona10 Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Lib/json/tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,7 @@ def main():


if __name__ == '__main__':
main()
try:
main()
except BrokenPipeError as exc:
sys.exit(exc.errno)
13 changes: 13 additions & 0 deletions Lib/test/test_json/test_tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import errno
import os
import sys
import textwrap
import unittest
import subprocess

from test import support
from test.support.script_helper import assert_python_ok

Expand Down Expand Up @@ -206,3 +208,14 @@ def test_ensure_ascii_default(self):
# asserting an ascii encoded output file
expected = [b'{', rb' "key": "\ud83d\udca9"', b"}"]
self.assertEqual(lines, expected)

@unittest.skipIf(sys.platform =="win32", "The test is failed with ValueError on Windows")
Copy link
Member

Choose a reason for hiding this comment

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

Where does the ValueError come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/python/cpython/pull/18779/checks?check_run_id=496883234

 test_broken_pipe_error (test.test_json.test_tool.TestTool) ... ERROR
  File "D:\a\cpython\cpython\lib\threading.py", line 944, in _bootstrap_inner
    self.run()
  File "D:\a\cpython\cpython\lib\threading.py", line 882, in run
    self._target(*self._args, **self._kwargs)
  File "D:\a\cpython\cpython\lib\subprocess.py", line 1493, in _readerthread
    buffer.append(fh.read())
ValueError: read of closed file
D:\a\cpython\cpython\lib\subprocess.py:1066: ResourceWarning: subprocess 6532 is still running
  _warn("subprocess %s is still running" % self.pid,
ResourceWarning: Enable tracemalloc to get the object allocation traceback
OSError: [Errno 22] Invalid argument

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:\a\cpython\cpython\lib\runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "D:\a\cpython\cpython\lib\runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "D:\a\cpython\cpython\lib\json\tool.py", line 76, in <module>
    main()
  File "D:\a\cpython\cpython\lib\json\tool.py", line 71, in main
    raise SystemExit(e)

Copy link

Choose a reason for hiding this comment

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

The test is failed with ValueError on Windows isn't grammatically correct. It should be The test failed with ValueError on Windows (minus the is) 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This PR is now closed. You can write a new PR to enhance the skip message.

def test_broken_pipe_error(self):
cmd = [sys.executable, '-m', 'json.tool']
proc = subprocess.Popen(cmd,
stdout=subprocess.PIPE,
stdin=subprocess.PIPE)
# bpo-39828: Closing before json.tool attempts to write into stdout.
proc.stdout.close()
proc.communicate(b'"{}"')
self.assertEqual(proc.returncode, errno.EPIPE)
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2020-03-05-00-57-49.bpo-39828.yWq9NJ.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix :mod:`json.tool` to catch :exc:`BrokenPipeError`. Patch by Dong-hee Na.