gh-155946: Improve error handling in SMTP email logging - #155950
gh-155946: Improve error handling in SMTP email logging#155950MazinSharaf wants to merge 7 commits into
Conversation
Refactor email message handling in SMTP logging to ensure proper cleanup and error handling.
| smtp.send_message(msg) | ||
| smtp.quit() | ||
| except Exception: | ||
| try: |
There was a problem hiding this comment.
Don't put it in an entire try-catch. Only the relevant parts.
There was a problem hiding this comment.
Address this. Don't use a full try-except block around code that doesn't need it.
| except Exception: | ||
| try: | ||
| smtp.close() | ||
| except Exception: | ||
| pass | ||
| raise |
There was a problem hiding this comment.
Only put a try-except when smtp is actually being used.
|
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 |
|
How do I add the NEWS entry? And also, am I able to use the original issuer's test? His code for reproducing was: |
Use https://blurb-it.herokuapp.com/ or write it yourself with
I beléieve so but you may also want to clean it up and have a minimal working example instead. You should also read the existing tests first. |
|
Do I just add the tests into the description of the PR or just leave a comment with the test(s)? |
Removed unnecessary blank lines in the SSL context setup.
I don't understand. The PR description isn't really relevant as long as we have the issue. |
|
You must add tests inside |
Add test for connection closure on SMTP send failure.
|
Sorry, got confused. Everything should be done now. |
…Ctsd4.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
| @@ -0,0 +1,2 @@ | |||
| :mod:`logging`: ensure that :class:`logging.handlesr.SMTPHandler` | |||
There was a problem hiding this comment.
| :mod:`logging`: ensure that :class:`logging.handlesr.SMTPHandler` | |
| :mod:`logging`: ensure that :class:`logging.handlers.SMTPHandler` |
Sorry there was a typo in my suggestion.
Fixes #155946
SMTPHandler.emit()leaks the SMTP connection when sending fails #155946