Fix issue 24#114
Conversation
Prevents a crash when the disk is full by catching OSError in add_request() and logging a warning instead of letting it propagate. Fixes ActivityWatch#8
Adds RequestQueue.wait_for_queue_empty() which blocks until all queued heartbeats have been dispatched, with an optional timeout. Also exposes it via ActivityWatchClient.wait_for_queue_empty() for convenience. Replaces the need for callers to use a hardcoded sleep() to wait for the queue to drain. Fixes ActivityWatch#24
| while self._persistqueue.qsize() > 0 or self._current is not None: | ||
| if timeout is not None and (datetime.now() - start_time).total_seconds() >= timeout: | ||
| return False | ||
| if self.wait(0.1): | ||
| # stop() was called while waiting | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Thread death leaves
wait_for_queue_empty spinning indefinitely
is_alive() is checked once at entry, but not inside the polling loop. If the RequestQueue thread dies unexpectedly (e.g., an unhandled exception propagates out of run()'s inner loops without setting _stop_event), self.wait(0.1) will keep returning False on timeout every 100 ms, qsize() and _current will never change, and any call with timeout=None will spin forever. Adding if not self.is_alive(): return False as the first check inside the loop would break out cleanly in that scenario.
| with mock.patch.object(client, "_post", slow_post): | ||
| rq.start() | ||
| rq.add_request("/api/0/buckets/test/heartbeat", {}) | ||
| result = rq.wait_for_queue_empty(timeout=0.5) | ||
| rq.stop() | ||
| rq.join() |
There was a problem hiding this comment.
rq.join() blocks for ~10 seconds in the timeout test
slow_post sleeps for 10 seconds unconditionally. After wait_for_queue_empty(timeout=0.5) returns, rq.stop() is called but the queue thread is already inside slow_post with no way to interrupt it. rq.join() then has to wait the remaining ~9.5 seconds before the thread exits. The test effectively adds ~10 s to the suite on every run. Using a much shorter sleep (e.g. 0.5 s) or a threading.Event that can be signaled by stop() would keep the test fast while still exercising the timeout path reliably.
| rq.stop() | ||
| rq.join() | ||
|
|
||
| assert result is False No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file. Both changed files (
client.py and test_requestqueue.py) are missing a trailing newline, which can cause noisy diffs and violates POSIX text file conventions.
| assert result is False | |
| assert result is False |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Problem
There's no way to reliably wait for the request queue to finish dispatching all queued heartbeats. The current workaround in the example watcher is a hardcoded
sleep(1), which is both fragile and wasteful.Fix
Added
RequestQueue.wait_for_queue_empty(timeout)which polls the queue until it's empty, with an optional timeout in seconds. Also exposed it asActivityWatchClient.wait_for_queue_empty(timeout)for convenience.Behavior:
Trueif the queue empties before the timeoutFalseif the timeout is reached firstTrueimmediately if the queue thread isn't runningTesting
test_wait_for_queue_empty_basic— queue drains normally while runningtest_wait_for_queue_empty_not_running— returns True immediately if thread not startedtest_wait_for_queue_empty_timeout— returns False when queue can't drain in timemypyclean, no newruffissues introducedFixes #24