gh-116267: Avoid formatting record twice in RotatingFileHandler. #122731
Conversation
|
Only two buildbot test failures, neither of which seems relevant to the change. |
| self._msg = None | ||
| else: | ||
| msg = self.format(record) | ||
| self.stream.write(msg + self.terminator) |
There was a problem hiding this comment.
FileHandler.emit() opens a stream if it was not open. If you want to use the cached formatted message, this should perhaps be done in FileHandler.emit() or StreamHandler.emit(). Or inline FileHandler.emit() and StreamHandler.emit() completely here. Or add parameter for formatted error message in FileHandler.emit() and StreamHandler.emit().
There was a problem hiding this comment.
FileHandler.emit() opens a stream if it was not open
True, but it's opened in shouldRollover(), so that part of FileHandler.emit() would never be exercised.
cpython/Lib/logging/handlers.py
Lines 196 to 197 in 9e551f9
There was a problem hiding this comment.
You are right. Although it may break subclasses that override shouldRollover() (if it returns early, before calling super().shouldRollover()).
Lib/logging/handlers.py
Outdated
| @@ -195,10 +202,11 @@ def shouldRollover(self, record): | |||
| """ | |||
| if self.stream is None: # delay was set... | |||
| self.stream = self._open() | |||
| pos = self.stream.tell() | |||
There was a problem hiding this comment.
You're right, it's related to a fix #116263 and can be removed here.
| else: | ||
| msg = self.format(record) | ||
| self.stream.write(msg + self.terminator) | ||
| self.flush() | ||
| except Exception: |
There was a problem hiding this comment.
This is not related to this issue, but there should be the RecursionError handler (see bpo-36272). And in many other places that call handleError().
I agree it's not pretty, but I'm not sure I'd go that far. Practicality beats purity? |
There was a problem hiding this comment.
In general LGTM. This is ugly, but I have no better ideas.
I have only two suggestions:
- I would prefer to merge gh-116263: Do not rollover empty files in RotatingFileHandler #122788 first. It is simpler and less contradictionary, I am going to backport it.
- It is worth to open a separate issue for RecursionError. It is bigger issue than just adding few
excepts.
| msg = self.format(record) | ||
| self.stream.write(msg + self.terminator) | ||
| self.flush() | ||
| except RecursionError: # See issue bpo-36272 |
There was a problem hiding this comment.
I think that it is better to open a separate issue for this. There may be other problems related to handling RecursionError here. For example, in SocketHandler we may want to close socket on RecursionError if closeOnError is true.
| self._msg = None | ||
| else: | ||
| msg = self.format(record) | ||
| self.stream.write(msg + self.terminator) |
There was a problem hiding this comment.
You are right. Although it may break subclasses that override shouldRollover() (if it returns early, before calling super().shouldRollover()).
OK, we can park this PR for now.
Fair enough. |
Uh oh!
There was an error while loading. Please reload this page.