bpo-33927: Add support for same infile and outfile to json.tool

[remilapeyre]

https://bugs.python.org/issue33927

[rhettinger]

This doesn't look like the right solution to me. You may need to skip the argparse.FileType() and move the opening and closing logic into the with-statements. That will allow "python3 -m json.tool example.json example.json" and fix the other minor fd leaks mentioned in the bug report.

[remilapeyre]

Hi @rhettinger, thanks for the comments.

fix the other minor fd leaks mentioned in the bug report.

I'm not sure to follow, if I understand Pablo's remark in https://bugs.python.org/issue33927#msg320326 the problem is about the leaked fd outfile when an exception is raised before the second with statement. This happens when both fd points to the same file, outfile being opened in 'w' mode, the content of the file gets destroyed before it could be read.

When this happens, the second fd has been open by argparse.FileType() but is never closed hence the leak. As I understand it, this never happens with the first fd since an exception can't be raised before the first with statement and it can therefore stay as argparse.FileType().

Both leaks shown by Pablo seems to be fixed by this patch:

➜  debug git:(bugfix/json.tool-same-infile-outfile) ✗ touch invalid_file.dat
➜  debug git:(bugfix/json.tool-same-infile-outfile) ✗ ./python.exe -m json.tool invalid_file.dat nofile.dat
Expecting value: line 1 column 1 (char 0)
➜  debug git:(bugfix/json.tool-same-infile-outfile) ✗ echo '{"foo":"bar"}'>valid.dat
➜  debug git:(bugfix/json.tool-same-infile-outfile) ✗ ./python.exe -m json.tool valid.dat valid.dat        
➜  debug git:(bugfix/json.tool-same-infile-outfile) ✗ cat valid.dat 
{
    "foo": "bar"
}

Am I missing something?

[4383]

Looks good to me!

$ wget https://raw.githubusercontent.com/remilapeyre/cpython/23593fb76c999ed96583b87d5d4873d5383215a2/Lib/json/tool.py
...
2018-06-26 21:58:05 (56,5 MB/s) - «tool.py» enregistré [1601/1601]
$ echo '{"foo": "bar"}' > test.json
$ python tool.py test.json 
{
    "foo": "bar"
}
$ python tool.py test.json test.json 
$ cat test.json 
{
    "foo": "bar"
}
$ echo '{"foo": "bar"}' | python tool.py 
{
    "foo": "bar"
}
$ python tool.py test.json /bla/test.json 
usage: python -m json.tool [-h] [--sort-keys] [infile] [outfile]
python -m json.tool: error: can't open '/bla/test.json': [Errno 2] No such file or directory: '/bla/test.json'

[4383]

The CI failed due to a timeout...

[remilapeyre]

Indeed, I ran it again and all is good now.

[4383]

Consider to reference these changes in a new entry to the Misc.
https://devguide.python.org/committing/#what-s-new-and-news-entries

[4383]

LGTM! Thanks for your contrib!

[remilapeyre]

Hi @rhettinger , I think test_no_fd_leak_infile_outfile and test_no_fd_leak_same_infile_outfile on c9d43c Lib/json/tool.py and 106f4b Lib/json/tool.py should convince you that this patch is enough to prevent leakage in both cases mentionned by pablogsal.

[matrixise]

Hi Rémi,

Thank you for your contribution but I think you need to improve your PR.
Have a nice day,

Thank you

[rhettinger]

Can you please fix the file conflicts?

[remilapeyre]

Hi @rhettinger, I resolved the conflix. One thing I changed is that infile is now read fully rather than one line at a time when json_lines is True (e606980#diff-e94945dd18482591faf1e294b029a6afR39) but I don't expect this to be an issue.

[remilapeyre]

@rhettinger, the resolved the conflicts with master, can you review the changes again?

[vstinner]

There is PR #11992 which is similar but looks simpler.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[vstinner]

There is PR #11992 which is similar but looks simpler.

Oh sorry, these two PR have a different purpose.

[4383]

Thoughts?

[vstinner]

What do you think about this?

I like -i option of the sed command: "sed -i file" modify file in-place, "sed file" writes the output into stdout.

man sed

       -i[SUFFIX], --in-place[=SUFFIX]

              edit files in place (makes backup if SUFFIX supplied)

I prefer to explicitly ask to modify the file in-place, rather than using black magic to try to support this uncommon case.

If -i/--in-place is used, you can read the full content in memory.

I disagree with changing the behavior (always load full data in memory) by default.

[remilapeyre]

Hi @vstinner, I made the changes requested to read infile only when --in-place is used. Can you review again?

[vstinner]

Hi @vstinner, I made the changes requested to read infile only when --in-place is used. Can you review again?

I don't have the bandwidth to review it. @matrixise @4383: Would you mind to review it.

@remilapeyre: Once someone will approve the PR, I will have a new look if you want ;-)

[4383]

Hi @vstinner, I made the changes requested to read infile only when --in-place is used. Can you review again?

I don't have the bandwidth to review it. @matrixise @4383: Would you mind to review it.

@remilapeyre: Once someone will approve the PR, I will have a new look if you want ;-)

@vstinner @remilapeyre Tomorrow I can review it and send my feed back.

[matrixise]

@remilapeyre Do you want to finish this PR, I could continue the review with you. Have a nice day.

[methane]

I close this because #29273 is merged.