Skip to content
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

Fix for bug 30378 regressed SysLogHandler by making it resolve addresses at initialization instead of in .emit() #82535

Open
ngie-eign mannequin opened this issue Oct 2, 2019 · 4 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@ngie-eign
Copy link
Mannequin

ngie-eign mannequin commented Oct 2, 2019

BPO 38354
Nosy @vsajip, @ngie-eign, @iritkatriel

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2019-10-02.21:32:12.533>
labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
title = 'Fix for bug 30378 regressed SysLogHandler by making it resolve addresses at initialization instead of in `.emit()`'
updated_at = <Date 2021-10-18.23:26:10.897>
user = 'https://github.com/ngie-eign'

bugs.python.org fields:

activity = <Date 2021-10-18.23:26:10.897>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2019-10-02.21:32:12.533>
creator = 'ngie'
dependencies = []
files = []
hgrepos = []
issue_num = 38354
keywords = []
message_count = 4.0
messages = ['353775', '353780', '353907', '404247']
nosy_count = 4.0
nosy_names = ['vinay.sajip', 'ngie', 'calcheng', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue38354'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

@ngie-eign
Copy link
Mannequin Author

ngie-eign mannequin commented Oct 2, 2019

The change made for bug 30378 caused a regression in our code by making
lookups for SysLogHandler addresses at init time, instead of making them more lazy. Example:

>>> import logging.handlers
>>> LOGGER = logging.getLogger("logger")
>>> LOGGER.addHandler(logging.handlers.SysLogHandler(address=('resolvessometimesbutnotthefirsttime.com', logging.handlers.SYSLOG_UDP_PORT)))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python2.7/logging/handlers.py", line 767, in __init__
    ress = socket.getaddrinfo(host, port, 0, socktype)
socket.gaierror: [Errno -2] Name or service not known

This is exacerbated by the fact that a lot of code the organization I work for (and to be honest, I have written in the past) initializes global loggers once at import time. If the SysLogHandler is added to the global logger and the DNS resolution isn't possible (/etc/hosts is empty on Unix or does not contain the entry, and DNS out is to lunch), it will fall on its face at initialization time.

There needs to be a more graceful way of initializing loggers like this, or a way of delaying the host resolution, so it fails gracefully and doesn't crash the entire program (restoring the previous behavior).

Example: SMTPHandler doesn't do name resolution until it calls emit, which seems like a much more logical place to do the operation (especially since DNS records can change or become invalid).

@ngie-eign ngie-eign mannequin added type-crash A hard crash of the interpreter, possibly with a core dump 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir labels Oct 2, 2019
@ngie-eign ngie-eign mannequin changed the title Fix for bug 30378 regressed SysLogHandler Fix for bug 30378 regressed SysLogHandler by making it resolve addresses at initialization instead of in .emit() Oct 2, 2019
@ngie-eign
Copy link
Mannequin Author

ngie-eign mannequin commented Oct 2, 2019

Capturing more context here, based on internal discussion: other handlers are doing address resolution in emit() (HTTPHandler, SMTPHandler), which is expensive. In order for SysLogHandler to not regress behavior and not become expensive, performance-wise, it would probably be best to use functools.lru_cache(), using the address and a timeout as the key when resolving the addresses to avoid always doing address resolutions, e.g., DNS lookups: https://docs.python.org/3/library/functools.html#functools.lru_cache .

@vsajip
Copy link
Member

vsajip commented Oct 4, 2019

This could perhaps be handled by just swallowing errors in the constructor (as is done for Unix sockets, for example) and trying to connect in emit() if there is no connection.

Not sure I want to go down the caching route - it complicates things for what is a rare use case, plus raises the question of allowing configurable timeouts for the cache, etc.

This seems a rare use case, so I have things that have a higher priority on my list of things to do, but I'll certainly review a patch to address this.

@iritkatriel
Copy link
Member

Reproduced on 3.11.

Changing type because crash typically refers to segfault or hang in the interpreter rather than a valid exception.

@iritkatriel iritkatriel added 3.10 only security fixes 3.11 only security fixes type-bug An unexpected behavior, bug, or error and removed 3.7 (EOL) end of life 3.8 only security fixes type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 18, 2021
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
Status: No status
Development

No branches or pull requests

2 participants