Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves worker resilience by adding error handling for handler exceptions and queue full scenarios. The changes ensure that worker threads continue operating even when individual handlers fail, and that the listener gracefully handles queue saturation.
Key changes:
- Worker threads now catch and log handler exceptions while ensuring
task_done()is always called - Queue full errors are caught and logged instead of crashing the listener
- Comprehensive test coverage for both resilience scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| telegram/worker.py | Wraps handler execution in try-except-finally to catch exceptions while ensuring task_done is always called |
| telegram/client.py | Adds try-except block around queue.put() to gracefully handle queue.Full exceptions with error logging |
| tests/test_telegram_methods.py | Adds tests for worker exception handling and queue full scenarios; includes code formatting improvements |
| docs/source/changelog.rst | Documents the new error handling features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/source/changelog.rst
Outdated
| - Python versions 3.7 and 3.8 are no longer supported. | ||
| - tdlib 1.8.31. | ||
| - Fix: Handle errors during updates processing | ||
| - Fix: Handle queye full errors |
There was a problem hiding this comment.
Typo in the changelog: "queye" should be "queue".
| - Fix: Handle queye full errors | |
| - Fix: Handle queue full errors |
tests/test_telegram_methods.py
Outdated
| # Give the worker time to process both items | ||
| time.sleep(1) |
There was a problem hiding this comment.
Using time.sleep(1) in tests can lead to flaky tests in slow CI environments. Consider using q.join() instead, which waits until all items in the queue have been processed (i.e., until task_done() has been called for each item). This would make the test more reliable and potentially faster.
| # Give the worker time to process both items | |
| time.sleep(1) | |
| # Wait until the worker has processed all items in the queue | |
| q.join() |
alexander-akhmetov
force-pushed
the
alexander-akhmetov/handler-errors
branch
2 times, most recently
from
176e02e to
a2a32a8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_telegram_methods.py
Outdated
| @@ -90,7 +93,10 @@ def test_parse_text_entities(self, telegram): | |||
|
|
|||
| def test_send_phone_number_or_bot_token(self, telegram): | |||
| # check that the dunction calls _send_phone_number or _send_bot_token | |||
There was a problem hiding this comment.
Spelling error: "dunction" should be "function".
| # check that the dunction calls _send_phone_number or _send_bot_token | |
| # check that the function calls _send_phone_number or _send_bot_token |
| def test_worker_continues_after_handler_exception(self): | ||
| """Handler exceptions should not kill the worker thread and task_done must be called""" | ||
| q = queue.Queue() | ||
| worker = SimpleWorker(q) | ||
| worker.run() | ||
|
|
||
| results = [] | ||
|
|
||
| def failing_handler(update): | ||
| raise ValueError("Handler failed") | ||
|
|
||
| def working_handler(update): | ||
| results.append(update) | ||
|
|
||
| # Put two items: one with a failing handler, one with a working handler | ||
| q.put((failing_handler, {"id": 1})) | ||
| q.put((working_handler, {"id": 2})) | ||
|
|
||
| # Give the worker time to process both items. | ||
| # Can't use join when the test fails. | ||
| time.sleep(1) | ||
|
|
||
| worker.stop() | ||
|
|
||
| assert results == [{"id": 2}] |
There was a problem hiding this comment.
The test claims to verify that task_done is called, but it doesn't actually assert this behavior. While the test checks that the worker continues processing after an exception (by verifying the second handler runs), it doesn't explicitly verify that task_done was called for the failing handler. Consider using a mock or spy on the queue to verify task_done is called twice (once for each item).
alexander-akhmetov
force-pushed
the
alexander-akhmetov/handler-errors
branch
from
a2a32a8 to
ef661bc
Compare
1 participant
No description provided.