[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Vinay Sajip]

Change by Vinay Sajip :


--
resolution:  -> not a bug
stage:  -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Irit Katriel]

Irit Katriel  added the comment:

This can be closed now - we all agree there is nothing left to do.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Irit Katriel]

Irit Katriel  added the comment:

The missing target error was fixed under issue41503

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Kostis Anagnostopoulos]

Kostis Anagnostopoulos  added the comment:

Have nothing else to say on top of what iritkatriel discovered, a double-check 
would be needed, but may be a pointless speed up, if MemoryHandler is 
impossible to work without a `target`.  

Fixing the no-target error is more important.  Actually that's how i came to 
notice this optimization.

--
status: pending -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Vinay Sajip]

Vinay Sajip  added the comment:

OK, I'll mark as pending and close in a few days if nothing more is heard from 
Kostis.

--
status: open -> pending

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Irit Katriel]

Irit Katriel  added the comment:

I think this issue should be closed - Kostis doesn't seem keen to discuss the 
merits of this change, and as I've said I don't think it's worth pursuing.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Irit Katriel]

Irit Katriel  added the comment:

I think it is unlikely that this change would make a measurable speedup 
(benchmarks would be needed to prove that it does).

And even if it does, it's for the case where no target has been set, in which 
case the logger isn't doing anything anyway. Why is this a case worth 
optimizing?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Irit Katriel]

Irit Katriel  added the comment:

Regarding the change you suggest here - I think you need add another check that 
target is not None after you've acquired the lock to make it safe.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Irit Katriel]

Irit Katriel  added the comment:

I guess the missing lock in setTarget is a different issue than the one you 
raise here and should be a separate ticket - I will create one.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Irit Katriel]

Irit Katriel  added the comment:

Actually the lock is needed and it is currently missing from setTarget. 

The script below causes handle to be called on None, causing:

  File "C:\Users\User\src\cpython\lib\logging\handlers.py", line 1265, in emit
self.flush()
  File "C:\Users\User\src\cpython\lib\logging\handlers.py", line 1348, in flush
self.target.handle(record)
AttributeError: 'NoneType' object has no attribute 'handle'



import io
import logging.handlers
import threading
import time

class SlowHandler:
def __init__(self):
self.stream = io.StringIO()
self.streamHandler = logging.StreamHandler(self.stream)

def handle(self, msg):
time.sleep(1)
self.streamHandler.handle(msg)

target = SlowHandler()
mem_hdlr = logging.handlers.MemoryHandler(10, logging.ERROR, target)
mem_logger = logging.getLogger('mem')
mem_logger.propagate = False
mem_logger.addHandler(mem_hdlr)

def toggleTarget():
time.sleep(1)
mem_hdlr.setTarget(None)

t = threading.Thread(target=toggleTarget, args=())
t.daemon = True
t.start()

while True:
time.sleep(0.1)
mem_logger.warning("warning not flushed")
mem_logger.error("error is flushed")
print("%s" % target.stream.getvalue().splitlines())



--
nosy: +iritkatriel

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41483] Do not acquire lock in MemoryHandler.flush() if no target defined

[Kostis Anagnostopoulos]

New submission from Kostis Anagnostopoulos :

The `logging.handlers.MemoryHandler.flush()` method acquire the lock even if 
target has not been set, and the method is a noop:

```
def flush(self):
# (Docstring skipped)
self.acquire()
try:
if self.target:
for record in self.buffer:
self.target.handle(record)
self.buffer..clear()
finally:
self.release()
```

An optimized version would re-arrrange the nesting to avoid the locking:

```
def flush(self):
# (Docstring skipped)
if self.target:
self.acquire()
try:
for record in self.buffer:
self.target.handle(record)
self.buffer.clear()
finally:
self.release()
```

- There is no use-case, beyond the expected performance gain.
- Without having searched, i would deem highly improbable the existence of code 
in the std-library or 3rdp code that depends on the current noop-locking when 
`flush()` is called.

--
components: Library (Lib)
messages: 374859
nosy: ankostis, penlect, vinay.sajip
priority: normal
severity: normal
status: open
title: Do not acquire lock in MemoryHandler.flush() if no target defined
type: performance
versions: Python 3.10, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com