[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
STINNER Victor added the comment: Sorry for not being more available for feedback on patches. I chose to write the final patch because patches were not updated to take in account my latest comments. I hope that this issue helps you at least the process for reviewing patches ;-) -- resolution: - fixed status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
Roundup Robot added the comment: New changeset 623e07ea43df by Victor Stinner in branch '3.4': Issue #23840: tokenize.open() now closes the temporary binary file on error to https://hg.python.org/cpython/rev/623e07ea43df New changeset a640d268ba97 by Victor Stinner in branch '3.5': (Merge 3.5) Issue #23840: tokenize.open() now closes the temporary binary file https://hg.python.org/cpython/rev/a640d268ba97 New changeset c5cfd6353b4b by Victor Stinner in branch 'default': (Merge 3.6) Issue #23840: tokenize.open() now closes the temporary binary file https://hg.python.org/cpython/rev/c5cfd6353b4b -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
STINNER Victor added the comment: You should see the new file in the next 30 minutes. I don't see the new file. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
STINNER Victor added the comment: tokenizeV2.patch and tokenize.patch have issues, I reviewed them. Can someone please write a new patch taking my comments in account? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
Matt Chung added the comment: Hey Haypo, I'm working on submitting the new patch now. Still getting used to the workflow and tools here. Thanks for being patient. You should see the new file in the next 30 minutes. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
STINNER Victor added the comment: tokenize.patch is wrong: you should not call buffer.close() if TextIOWrapper() was created successfully. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
Changes by Serhiy Storchaka storch...@gmail.com: -- components: +Library (Lib) stage: - needs patch type: - resource usage ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
STINNER Victor added the comment: I sent an email to the python core mentorship mailing list to find a volunteer to fix this easy issue. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
New submission from STINNER Victor: If TextIOWrapper constructor fails in tokenize.open(), the open binary file is not closed and a ResourceWarning is emited. Try attached patch: $ python3 -Wd ~/tokenize_open_bug.py tokenize.open failed: unknown encoding for 'test.py': xxx /home/haypo/tokenize_open_bug.py:13: ResourceWarning: unclosed file _io.BufferedReader name='test.py' print(tokenize.open failed: %s % err) The fix is quite simple: add try: ... except: buffer.close(); raise. If someone wants to fix this issue, an unit test must be added, test based on my script and ensures that the buffer is closed (ex: mock tokenize._builtin_open and checks that close() was called). -- files: tokenize_open_bug.py keywords: easy messages: 239786 nosy: haypo priority: normal severity: normal status: open title: tokenize.open() leaks an open binary file on TextIOWrapper error versions: Python 3.4, Python 3.5 Added file: http://bugs.python.org/file38781/tokenize_open_bug.py ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
Matt Chung added the comment: Updated patch based off of haypo's comment. Changed except Exception as err: - except -- Added file: http://bugs.python.org/file38795/tokenize.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
shiyao.ma added the comment: Based upon the previous review. Catch the TextWrapper, move the test into a function. -- Added file: http://bugs.python.org/file38796/tokenizeV2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
Matt Chung added the comment: Based off of storchaka's comment, moving the -- Added file: http://bugs.python.org/file38799/tokenize.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
Matt Chung added the comment: I believe it goes here under the TestTokenize class. Going to give it a shot now. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
STINNER Victor added the comment: ITSM it's not the TextIOWrapper but the detect_encoding fails and throws an error. Oh, right. But TextIOWrapper can fail for differen reasons. For example, CTRL+c may send KeyboardInterrupt. Try for example: with unittest.mock.patch.object(tokenize, '_builtin_open') as mock_open: mock_file = mock_open.return_value mock_file.tell.side_effect = OSError mock_file.readline.return_value = b'' tokenize.open(fn) This example raises an OSError in TextIOWrapper on file.tell(), and file.close() is not called. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
Matt Chung added the comment: Comparing to introom. Any reason to not wrap the entire in a try? Attached patch. -- nosy: +itsmemattchung Added file: http://bugs.python.org/file38793/issue23840.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
Matt Chung added the comment: Curious, @haypo, are you looking for a new unittest or a function within an existing unit test? Perhaps go under TestTokenize? 1227 def test_main(): 1228 from test import test_tokenize 1229 support.run_doctest(test_tokenize, True) 1230 support.run_unittest(TestTokenizerAdheresToPep0263) 1231 support.run_unittest(Test_Tokenize) 1232 support.run_unittest(TestDetectEncoding) 1233 support.run_unittest(TestTokenize) 1234 support.run_unittest(UntokenizeTest) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
shiyao.ma added the comment: ITSM it's not the TextIOWrapper but the detect_encoding fails and throws an error. So I wrapped a try/catch block around that. -- keywords: +patch nosy: +introom Added file: http://bugs.python.org/file38792/tokenize.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
shiyao.ma added the comment: On Wed, Apr 1, 2015 at 6:18 PM, STINNER Victor rep...@bugs.python.org wrote: Oh, right. But TextIOWrapper can fail for differen reasons. For example, CTRL+c may send KeyboardInterrupt. Yes. Any comment on the test? I wil update a second patch. Regards. -- 吾輩は猫である。ホームーページはhttp://introo.me。 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23840] tokenize.open() leaks an open binary file on TextIOWrapper error
STINNER Victor added the comment: Yes. Any comment on the test? Yes, as I wrote, I reviewed patches. You may get a notification by email. Anyway, it's here: http://bugs.python.org/review/23840/diff/14417/Lib/test/test_tokenize.py (I also commented tokenize.py) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23840 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
