Sitelet https://web.archive.org/web/20210726141213/https://github.com/python/cpython/pull/27310
Skip to content

/ cpython

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.

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

bpo-42378: fixed log truncation on logging shutdown #27310

Merged
merged 7 commits into from Jul 25, 2021
Merged

bpo-42378: fixed log truncation on logging shutdown #27310

merged 7 commits into from Jul 25, 2021
+32 −6

Conversation

@akulakov
Copy link
Contributor

@akulakov akulakov commented Jul 23, 2021 •
edited by miss-islington

https://bugs.python.org/issue42378

Automerge-Triggered-By: GH:vsajip
akulakov added 2 commits Jul 23, 2021
@akulakov
fix log truncation on logging shutdown; add unit test
198c6dc
@akulakov
add issue comment
4f14346
@akulakov akulakov requested a review from vsajip as a code owner Jul 23, 2021
@akulakov akulakov changed the title bpo-72378: fixed log truncation on logging shutdown bpo-42378: fixed log truncation on logging shutdown Jul 23, 2021
@vsajip
vsajip requested changes Jul 25, 2021
View changes
Copy link
Member

@vsajip vsajip left a comment

Generally looks OK, but I would suggest the following changes:

  • _closed = True is done in Handler.close(), and so there is a reliance that FileHandler.close() will call the superclass' close(). Of course it does, and there is a comment there about bpo-19523 and avoiding a handler leak, but I would add under that something like 
    # See also bpo-42378: we also rely on
# self._closed being set to True there
  • As you've added next_rec() to BaseFileTest, it can be removed from RotatingFileHandlerTest, which inherits from BaseFileTest.
  • And, of course, add the documentation update and NEWS entry, as you've mentioned on the issue,

Thanks!
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 25, 2021

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.
akulakov added 4 commits Jul 25, 2021
@akulakov
add comments; add news entry; remove duplicate add_rec
73c658b
@akulakov
Merge branch 'main' of github.com:akulakov/cpython into 42378-Fix-log…
2748528 
…-truncation-atexit
@akulakov
updated docs
1fad5d4
@akulakov
fix issue link
f659b7f
@akulakov
Copy link
Contributor Author

@akulakov akulakov commented Jul 25, 2021

@vsajip Thanks for the review!
I pushed an update per comments, PTAL..
@vsajip
vsajip requested changes Jul 25, 2021
View changes
Copy link
Member

@vsajip vsajip left a comment

Just minor changes suggested to the documentation part. Thanks!
Doc/library/logging.handlers.rst Outdated
@@ -117,6 +117,9 @@ sends logging output to a disk file. It inherits the output functionality from

Outputs the record to the file.

Note that if the file was closed due to logging shutdown at exit and file
mode is 'w', record will not be emitted (see :issue:`42378`).

This comment has been minimized.

Sign in to view
@vsajip

vsajip Jul 25, 2021
Member

I would change "file mode" to "the file mode" and "record" to "the record".
@akulakov
small update to docs
30cf315
@akulakov
Copy link
Contributor Author

@akulakov akulakov commented Jul 25, 2021

@vsajip fixed the docs per comment..
@vsajip
vsajip approved these changes Jul 25, 2021
View changes
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 25, 2021

@akulakov: Status check is done, and it's a success ✅ .
@miss-islington miss-islington merged commit 96cf5a6 into python:main Jul 25, 2021
13 checks passed
13 checks passed
@github-actions
Docs
Details
@github-actions
Check for source changes
Details
@github-actions
Check if generated files are up to date
Details
@github-actions
Windows (x86)
Details
@github-actions
Windows (x64)
Details
@github-actions
macOS
Details
@github-actions
Ubuntu
Details
@github-actions
Ubuntu SSL tests with OpenSSL
Details
@github-actions
Address sanitizer Address sanitizer
Details
Azure Pipelines PR #20210725.21 succeeded
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@bedevere-bot
bedevere/issue-number Issue number 42378 found
Details
@bedevere-bot
bedevere/news News entry found in Misc/NEWS.d
@akulakov
Copy link
Contributor Author

@akulakov akulakov commented Jul 25, 2021

thanks @vsajip !
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsajip vsajip

Assignees
No one assigned
Labels
🤖 automerge CLA signed
Projects
None yet
Milestone
No milestone
5 participants
@akulakov @bedevere-bot @miss-islington @vsajip @the-knights-who-say-ni