Skip to content

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

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

Jump to bottom
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

Conversation

akulakov
Copy link
Contributor

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

https://bugs.python.org/issue42378

Automerge-Triggered-By: GH:vsajip

@akulakov akulakov requested a review from vsajip as a code owner July 23, 2021 16:36
@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 reviewed changes
Copy link
Member

@vsajip vsajip left a comment

Choose a reason for hiding this comment

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

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

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
Copy link
Contributor Author

@vsajip Thanks for the review!
I pushed an update per comments, PTAL..

vsajip
vsajip requested changes Jul 25, 2021
View reviewed changes
Copy link
Member

@vsajip vsajip left a comment

Choose a reason for hiding this comment

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

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`).
Copy link
Member

Choose a reason for hiding this comment

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

I would change "file mode" to "the file mode" and "record" to "the record".

@akulakov
Copy link
Contributor Author

@vsajip fixed the docs per comment..

@miss-islington
Copy link
Contributor

@akulakov: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 96cf5a6 into python:main Jul 25, 2021
@akulakov
Copy link
Contributor Author

thanks @vsajip !

@vsajip vsajip added the needs backport to 3.10 only security fixes label Jan 7, 2022
@miss-islington
Copy link
Contributor

Thanks @akulakov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jan 7, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 7, 2022
@akulakov @miss-islington
bpo-42378: fixed log truncation on logging shutdown (pythonGH-27310)
94b7210 
Automerge-Triggered-By: GH:vsajip
(cherry picked from commit 96cf5a6)

Co-authored-by: andrei kulakov <[email protected]>
@bedevere-bot
Copy link

GH-30468 is a backport of this pull request to the 3.10 branch.

vsajip pushed a commit that referenced this pull request Jan 7, 2022
@miss-islington @akulakov
[3.10] bpo-42378: fixed log truncation on logging shutdown (GH-27310) (…
e35430b 
…GH-30468)

Co-authored-by: andrei kulakov <[email protected]>
@ecatmur ecatmur mannequin mentioned this pull request May 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsajip vsajip vsajip approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@akulakov @bedevere-bot @miss-islington @vsajip @the-knights-who-say-ni