Skip to content

Fix threading block at exit - #11

Merged
Shmuma merged 15 commits into
mainfrom
fix-threads
Oct 1, 2026
Merged

Shmuma merged 15 commits into
mainfrom
fix-threads

Conversation

@Shmuma

@Shmuma Shmuma commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Closes #10

@Shmuma
Shmuma deployed to manual-approval September 28, 2026 06:38 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 28, 2026 06:46 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 28, 2026 06:50 — with GitHub Actions Active
@Shmuma
Shmuma marked this pull request as ready for review September 28, 2026 06:50

@ahsimb ahsimb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread exasol/telemetry/client/setup.py
Comment thread exasol/telemetry/client/setup.py
Comment thread exasol/telemetry/client/setup.py
Comment thread exasol/telemetry/client/setup.py
Comment thread exasol/telemetry/client/setup.py
Comment thread exasol/telemetry/client/setup.py
Comment thread exasol/telemetry/client/setup.py
Comment thread test/unit/client/test_setup.py
@Shmuma

Shmuma commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

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.

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.

@Shmuma
Shmuma deployed to manual-approval September 29, 2026 06:46 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 29, 2026 06:55 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 29, 2026 06:57 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 29, 2026 06:58 — with GitHub Actions Active
@Shmuma
Shmuma requested a review from ahsimb September 29, 2026 06:58
@Shmuma
Shmuma deployed to manual-approval September 29, 2026 11:25 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 29, 2026 13:44 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 30, 2026 03:54 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 30, 2026 03:55 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 30, 2026 12:19 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval September 30, 2026 12:21 — with GitHub Actions Active
@Shmuma
Shmuma deployed to manual-approval October 1, 2026 05:53 — with GitHub Actions Active
@sonarqubecloud

sonarqubecloud Bot commented Oct 1, 2026

Copy link
Copy Markdown

@Shmuma
Shmuma merged commit e0fb038 into main Oct 1, 2026
17 checks passed
@Shmuma
Shmuma deleted the fix-threads branch October 1, 2026 08:29

This branch was successfully deployed

1 active deployment
manual-approval — e387f8a2 Deployed Oct 1, 2026 by Shmuma via Merge Gate / Approve Running Slow Tests? #61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thread blocks the python exit

2 participants