8000 gh-126400: Add TCP socket timeout to SysLogHandler to prevent blocking by hodamarr · Pull Request #126716 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-126400: Add TCP socket timeout to SysLogHandler to prevent blocking #126716

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 5 commits into from
Jan 29, 2025
Merged

gh-126400: Add TCP socket timeout to SysLogHandler to prevent blocking #126716

merged 5 commits into from
Jan 29, 2025

Conversation

hodamarr
Copy link
Contributor
@hodamarr hodamarr commented Nov 12, 2024 •
edited by bedevere-app bot
Loading

Issue #126400.
branch name - fix-issue-126400

Summary

In the current implementation of the SysLogHandler class from the logging.handlers module, if the specified address does not respond or is unreachable, the createSocket method may block indefinitely in case of using TCP socket (based on stream). This is problematic because there is no way to set a socket timeout during the connection's creation without overriding the entire createSocket method.

Changes

  1. Added a timeout parameter to the SysLogHandler.__init__ method, allowing users to specify a timeout for the socket connection.
  2. Updated the createSocket method to apply the timeout to the socket if provided.

This change allows users to prevent indefinite blocking on socket creation, enabling more robust error handling when the SysLog server is unresponsive.

Example Usage

With this update, users can now create a SysLogHandler with a timeout as follows:

import logging
from logging.handlers import SysLogHandler

# Create a SysLogHandler with a 5-second timeout
handler = SysLogHandler(address=('localhost', 514), timeout=5)

@hodamarr hodamarr requested a review from vsajip as a code owner November 12, 2024 07:05
@ghost
Copy link
ghost commented Nov 12, 2024 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link
bedevere-app bot commented Nov 12, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app bedevere-app bot mentioned this pull request Nov 12, 2024
Closed
@vsajip
Copy link
Member
vsajip commented Nov 12, 2024

Thank you for the PR. Would you mind updating the documentation, adding a NEWS entry using the blurb tool and signing the CLA? Thanks again.

@hodamarr
Copy link
Contributor Author

Hi @vsajip, added

@vsajip
Copy link
Member
vsajip commented Nov 12, 2024

Thanks for those. You don't feel comfortable updating the docs?

@hodamarr
Copy link
Contributor Author
hodamarr commented Nov 12, 2024 •
edited
Loading

@vsajip Oh, I thought you meant only the blurb. I'll update the documentation as well
This is the first time that I'm contributing to open source :)

@vsajip
Copy link
Member
vsajip commented Nov 13, 2024

This is the first time that I'm contributing to open source

Hopefully the first of many contributions! As it's an API change, it needs the documentation updating so users will know about the new parameter.

asvetlov
asvetlov reviewed Nov 13, 2024
View reviewed changes
Misc/NEWS.d/next/Library/2024-11-12-15-55-14.gh-issue-126400.ztsnx5.rst Outdated
@@ -0,0 +1,3 @@
Added a timeout parameter to logging.handlers.SysLogHandler class to control the TCP socket timeout and prevent process blocking when the Syslog server is unreachable; this improves resilience when working with unresponsive Syslog servers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Added a timeout parameter to logging.handlers.SysLogHandler class to control the TCP socket timeout and prevent process blocking when the Syslog server is unreachable; this improves resilience when working with unresponsive Syslog servers.
Added a timeout parameter to :class:`logging.handlers.SysLogHandler` class to control the TCP socket timeout and prevent process blocking when the Syslog server is unreachable; this improves resilience when working with unresponsive Syslog servers.

asvetlov
asvetlov requested changes Nov 13, 2024
View reviewed changes
Copy link
Contributor
@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Please update file ./Doc/library/logging.handlers.rst to mention added timeout parameter.

Take a look atsocktype argument for inspiration.

@bedevere-app
Copy link
bedevere-app bot commented Nov 13, 2024

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.

@bedevere-app
Copy link
bedevere-app bot commented Nov 30, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

vsajip
vsajip reviewed Jan 19, 2025
View reviewed changes
@bedevere-app
Copy link
bedevere-app bot commented Jan 19, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link
bedevere-app bot commented Jan 29, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@vsajip
Copy link
Member
vsajip commented Jan 29, 2025

The docs build is failing with some unrelated error in processing the Misc/NEWS entries, same happens locally. I've fixed the sphinx lint error and missing blurb in this PR. Not sure what the docs build problem is.

@vsajip
Copy link
Member
vsajip commented Jan 29, 2025

Somewhere, perhaps during a force-push, the original blurb got lost. I added a new one (a bit shorter) and everything now seems to be building correctly. I had to update a couple of unrelated blurb files, seemingly because they wrongly had gh: instead of gh-issue: in places (perhaps a change to blurb caused the build failures).

@vsajip vsajip merged commit fdcedfd into python:main Jan 29, 2025
41 of 42 checks passed
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025
@hodamarr @vsajip @srinivasreddy
pythongh-126400: Add TCP socket timeout to SysLogHandler to prevent b…
f10f690 
…locking (pythonGH-126716)

Co-authored-by: Vinay Sajip <vinay_sajip@yahoo.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvetlov asvetlov asvetlov requested changes

@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.
3 participants
@hodamarr @vsajip @asvetlov
0