Repository navigation
Conversation
When a buffer_size is configured, lines are held in the LineBuffer until it fills up or the background thread flushes it. SocketWriter.close() closed the socket without sending what was still buffered, so those lines were lost, and the background thread then kept failing to send them on the closed socket. Send any buffered lines before closing.
| with self._lock: | ||
| if self._buffer is not None and len(self._buffer) > 0: | ||
| try: | ||
| self._sock.sendto(bytes(self._buffer.flush(), encoding="utf-8"), self._address) |
There was a problem hiding this comment.
If this throws something other than IOError, e.g. UnicodeEncodeError, then the socket won't be closed.
There was a problem hiding this comment.
Good catch, thanks. In f3faac1 the flush in close() now sits in a try/finally, so the socket is always closed, even if the send raises something like UnicodeEncodeError (the error still propagates after the socket is closed). I added a test that buffers a lone surrogate and checks the socket is closed after close() raises.
| if self._sock is None: | ||
| return | ||
| self._sock.close() | ||
| with self._lock: |
There was a problem hiding this comment.
close() can deadlock if it runs from a signal handler
This change makes close() take self._lock, a plain threading.Lock. _write_buffer holds the same lock while it appends and, when the buffer fills, while it calls sendto.
Python runs signal handlers on the main thread, between bytecode instructions. That can happen while the main thread is inside with self._lock: in _write_buffer. A common graceful-shutdown pattern is:
def handle_sigterm(signum, frame):
registry.close()
sys.exit(0)
signal.signal(signal.SIGTERM, handle_sigterm)If SIGTERM arrives while the main thread is recording a metric (with buffer_size > 0):
- The main thread is in _write_buffer and holds _lock.
- The signal handler runs on that same thread and calls registry.close(), which calls SocketWriter.close().
- close() runs with self._lock:. The lock is not reentrant and its owner is the thread now waiting for it, so this blocks forever.
The process hangs at shutdown instead of exiting, and it keeps hanging until something sends SIGKILL.
There was a problem hiding this comment.
Thanks for the detailed walkthrough, you are right. In f3faac1 the lock is now a threading.RLock, so a close() from a signal handler on the thread that is already inside _write_buffer can take the lock again instead of blocking forever. _write_buffer keeps a local reference to the socket, so if the handler closes it and returns, the pending send only fails with an OSError that is already caught and logged. There is a new test that calls close() while the same thread holds the lock and checks it returns.
| time.sleep(5) | ||
| if self._buffer is None or self._sock is None: | ||
| continue | ||
| if len(self._buffer) > 0: |
There was a problem hiding this comment.
I think we may need to recheck this after the lock is acquired. Otherwise it close happens in between we close and log an error.
Related, do we need a closed flag or maybe setting self._sock to None in close.
There was a problem hiding this comment.
Agreed. In f3faac1 close() sets a _closed flag and swaps self._sock to None under the lock. The background flush and _write_buffer recheck the socket after taking the lock, _acquire_socket does not create a new socket once the writer is closed, and the background thread loop stops after close.
Use an RLock so close() can run from a signal handler while the same thread holds the lock in _write_buffer, close the socket in a finally block so a non IOError exception cannot leak it, and mark the writer as closed by setting _sock to None so the background flush and later writes recheck it under the lock instead of using a closed socket.
2 participants
When a
buffer_sizeis configured,SocketWriter.close()closed the socket without sending the lines still held in theLineBuffer, so those metrics were dropped and the background flush thread kept failing on the closed socket. This sends any buffered lines before closing. A UDP test covers the buffered close path.