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
logging.handlers.QueueHandler should not lock when handling a record #68833
Comments
The Queue backing the QueueHandler is already sufficiently locking for thread-safety. This isn't a huge issue, but the QueueHandler is a very nice built-in which could be used to work around a deadlock I've encountered several times. In brief, functions which can cause other threads to log being called from either __repr__() or __str__(). |
Can you expand on the deadlock? Are you saying that the "extra" locking is causing the deadlock? |
On 16/07/15 20:03 +0000, R. David Murray wrote:
Sure, here is the simplest example of the deadlock I could come up with. Using What happens is this:
import logging
import logging.handlers
import queue
import types
import threading
fmt = logging.Formatter('LOG: %(message)s')
stream = logging.StreamHandler()
stream.setFormatter(fmt)
log_queue = queue.Queue(-1)
queue_handler = logging.handlers.QueueHandler(log_queue)
queue_listener = logging.handlers.QueueListener(log_queue, stream)
queue_listener.start()
def handle(self, record):
rv = self.filter(record)
if rv:
self.emit(record)
return rv
# Uncomment to remove deadlock
#queue_handler.handle = types.MethodType(handle, queue_handler)
logger = logging.getLogger()
logger.addHandler(queue_handler)
logger.setLevel(logging.DEBUG)
class Blah(object):
def __init__(self):
self._in = queue.Queue()
self._out = queue.Queue()
def pub():
self._in.get(block=True)
logging.info('Got a request')
self._out.put('hi')
self._pub_thread = threading.Thread(target=pub)
self._pub_thread.start()
def __repr__(self):
self._in.put('gimme data')
return self._out.get()
def __del__(self):
self._pub_thread.join()
b = Blah()
logger.info('About to log')
logger.info('blah = %s', b)
queue_listener.stop() |
I can't see doing io in __repr__ ever making sense, so I'm not sure this is a use case we care about. But Vinay might not have any objection to removing locking if it is redundant, so we'll see what he has to say. |
I'm not sure I want to make a special case just to support what seems like a somewhat pathological use case (no offence intended). If you need this, there's no reason you couldn't subclass QueueHandler and override handle(), is there? |
On 16/07/15 23:21 +0000, Vinay Sajip wrote:
Hey, no offense taken, it wasn't even my code that tripped on this, I Anyways, it's up to you all if you want to include this, as you However, the reason I submitted the patch is because I believe using the |
I agree the lock could be seen as redundant as there are no shared data structures used by the current implementation of the enqueue and prepare methods (which are called after the lock is acquired), but users could potentially override those methods in subclasses in ways that require locking to be thread-safe. Currently, people overriding emit() [or code called from there] know that any state they manipulate there is protected by the handler lock - and this would not be the case if I implemented your suggestion. It could certainly break subclasses of QueueHandler which are out there in the wild. Note that the reference to locking is there in the Handler.handle docstring - https://docs.python.org/library/logging.html#logging.Handler.handle - so changing it would change the contract which is documented. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: