Fix threading block at exit - #11
Conversation
ahsimb
left a comment
There was a problem hiding this comment.
The core fix for #10 looks right: making the worker a daemon thread means Python no longer waits for it at shutdown, and the atexit handler still sends the remaining data and stops the worker cleanly. Previously the interpreter joined the non-daemon worker before running atexit handlers, so the handler that would have stopped the worker never ran.
My main concern is the new re-configuration logic in setup() (second call with a different disable value). It isn't needed to fix #10, and it brings in several problems (see inline comments). Since setup() isn't part of the public API (__init__ exports only track, disable and shutdown) and the internal caller _do_setup() passes no arguments, the simplest option might be to drop that change. If it's needed, please fix the inline points and add tests for it.
One more thing, not in this diff but related to blocking: in worker.track() (worker.py ~line 310), if _queue.not_full: tests a threading.Condition object, which is always truthy, so the check never skips. If the worker is stuck in a slow requests.post and more than MAX_QUEUE_CAPACITY (10) messages are tracked, the next track() blocks the application thread until the send times out. Suggest _queue.put_nowait(...) with except queue.Full: pass, maybe as a separate issue.
Overall observation is correct, setup is not exposed at the moment. The fix was needed, as one of the prior tests were broken because of the wrong logic. But at the same time, we might need a way to explicitly configure the telemetry in the future, so I think setup could be seen not as "private", but rather as "protected" API. Will add the fix description to the release notes to track the fix. |
|



Closes #10