Skip to content

gh-116267: Do not check record size in RotatingFileHandler to avoid f… #116330

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

Closed
wants to merge 4 commits into from

Conversation

Jason-Y-Z
Copy link
Contributor

@Jason-Y-Z Jason-Y-Z commented Mar 4, 2024

…ormatting twice

This stops us from formatting the new record twice in RotatingFileHandler.

@serhiy-storchaka
Copy link
Member

@vsajip, what are your thoughts about this?

Comment on lines +6151 to +6152
self.assertFalse(rh.shouldRollover(self.next_rec()))
rh.emit(self.next_rec())
Copy link
Member

Choose a reason for hiding this comment

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

self.next_rec() returns a new record. You should save it and pass the same record to shouldRollover() and emit().

@Jason-Y-Z Jason-Y-Z closed this Aug 3, 2024
@Jason-Y-Z Jason-Y-Z deleted the fix-issue-116263 branch August 3, 2024 19:40
@vsajip
Copy link
Member

vsajip commented Aug 5, 2024

I'm not sure why @Jason-Y-Z closed this - maybe due to impatience due to my lack of response. I've not had much time to look at this, but I was thinking about trying a different approach - caching the formatted value in an instance variable to avoid reformatting. Since everything happens inside emit(), in theory one could avoid coupling by replacing the FileHandler.emit(...) call with just the stream.write(...) and self.flush() that StreamHandler.emit() does (since self.stream won't be None at that point, FileHandler.emit() is essentially the same as StreamHandler.emit(...) - it's just two lines of code duplication, and so doesn't seem unreasonable to do to avoid coupling. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants