bpo-39828: Fix json.tool to catch BrokenPipeError. #18779
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
remilapeyre
reviewed
Mar 4, 2020
|
I expect the same behavior as this command. |
I'm not aware of any tool which logs an error into stderr when stdout pipe is closed, before the producer has time to write all its output into stdout. |
vstinner
approved these changes
Mar 4, 2020
There was a problem hiding this comment.
LGTM.
It seems like it's the first time that we start to catch explicitly BrokenPipeError in Python stdlib. Only the subprocess has code to handle its own pipes, which is a more specific case.
I'm not sure if it's the right approach, so I would prefer to get a review from another core dev.
cc @pablogsal @methane @serhiy-storchaka: Does it look like the good design to you to handle BrokenPipeError?
At least ➜ ~ set -o pipefail
➜ ~ echo "{}" | wc -l | true
➜ ~ echo $?
141
➜ ~ echo foo | cat | true
➜ ~ echo $?
141
➜ cpython git:(master) git log | true
➜ cpython git:(master) echo $?
141
➜ ~ echo '{}' | jq '.' | true
➜ ~ echo $?
141
➜ ~ seq 1 10000 | true
➜ ~ echo $?
141and curl sets a non zero exit code and write an error on stderr: ➜ ~ curl -s https://google.com | true
(23) Failed writing body
➜ ~ echo $?
23This change make ➜ cpython git:([bpo-39828](https://bugs.python.org/issue39828)) echo "{}" | ./python.exe -m json.tool | true
➜ cpython git:([bpo-39828](https://bugs.python.org/issue39828)) echo $?
0I'm not sure where the empty line comes from. I think we should at least set a non zero exit code. |
By the way, Python exits with code 120 if it fails to flush stdout or stderr: Change introduced in bpo-5319 by the commit b4ce1fc. The issue uses |
|
I've updated the PR. ➜ cpython git:([bpo-39828](https://bugs.python.org/issue39828)) ✗ echo "{}" | ./python.exe -m json.tool | true
➜ cpython git:([bpo-39828](https://bugs.python.org/issue39828)) ✗ echo $?
141 |
|
But this solution is not proper for Windows. According to mypy issue, python/mypy#2893 |
|
FYI, 7259ef6 returns cpython git:([bpo-39828](https://bugs.python.org/issue39828)) ✗ echo "{}" | ./python.exe -m json.tool | true
➜ cpython git:([bpo-39828](https://bugs.python.org/issue39828)) ✗ echo $?
32 |
How, that's interesting. I think it doesn't work here since the pipe is closed when
@corona10, thanks for looking into this issue, 7259ef675a4682118d5967437591569c27415984 looks good indeed 👍 |
remilapeyre
approved these changes
Mar 6, 2020
vstinner
reviewed
Mar 6, 2020
Misc/NEWS.d/next/Library/2020-03-05-00-57-49.bpo-39828.yWq9NJ.rst
Outdated
Show resolved
Hide resolved
corona10
changed the title
corona10
changed the title
|
Thanks for the review :) |
vstinner
approved these changes
Mar 7, 2020
methane
approved these changes
Mar 7, 2020
|
Is it possible to write a reliable test for BrokenPipeError? Maybe using a string longer than support.test.PIPE_MAX_SIZE to ensure that the pipe is full? |
|
While I tried with this code, the test was not failed... (This test should be failed if my assumption is right.) def test_broken_pipe_error(self):
cmd = [sys.executable, '-m', 'json.tool']
large_data = 'xxx' * support.PIPE_MAX_SIZE
large_data = f'"{large_data}"'
process = subprocess.run(cmd, input=large_data, capture_output=True, text=True, check=True)
self.assertEqual(process.stdout.strip(), large_data)
self.assertEqual(process.stderr, '') |
|
Did you try using Popen and closing the stdout pipe? |
Oh It works :) Greeat! Thank you |
corona10
changed the title
vstinner
reviewed
Mar 9, 2020
| @@ -206,3 +207,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") | |||
There was a problem hiding this comment.
Where does the ValueError come from?
There was a problem hiding this comment.
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)
|
Sorry, @corona10 and @vstinner, I could not cleanly backport this to |
|
@corona10: This fix deserves to be backported to 3.7 and 3.8, but automated backport failed. Can you please try to do the backport manually? |
|
Sure :) |
corona10
added a commit
to corona10/cpython
that referenced
this pull request
Mar 10, 2020
…). (cherry picked from commit 700cb58) Co-authored-by: Dong-hee Na <[email protected]>
|
GH-18894 is a backport of this pull request to the 3.8 branch. |
corona10
added a commit
to corona10/cpython
that referenced
this pull request
Mar 10, 2020
…). (cherry picked from commit 700cb58) Co-authored-by: Dong-hee Na <[email protected]>
miss-islington
pushed a commit
that referenced
this pull request
Mar 10, 2020
|
|
miss-islington
pushed a commit
that referenced
this pull request
Mar 10, 2020
|
|
bl-ue
reviewed
May 16, 2021
| @@ -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") | |||
There was a problem hiding this comment.
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) 🤔
There was a problem hiding this comment.
This PR is now closed. You can write a new PR to enhance the skip message.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugs.python.org/issue39828