fix(odds-ticker): stop the marquee freezing on its own background work - #469
Merged
Conversation
display() runs on the shared display loop, so anything it waits on freezes
the whole rotation. Two paths in it were written as a thread immediately
followed by a blocking get():
t = threading.Thread(target=work); t.start()
q.get(timeout=5) # <-- on the render thread
which is a blocking call wearing a thread as a disguise. The thread bought
nothing, and could not have helped anyway: the work is requests and PIL, so
it holds the GIL for its Python parts.
Measured on hardware over 45 minutes, odds-ticker's frame statistics matched
the other scrolling plugins almost exactly -- 0.14% stalls, 10.00ms median --
yet it visibly stuttered where they did not. The medians hid it. The maxima
did not: single frames of 981ms, 1010ms, 2202ms and 4829ms, with the queue
timeouts (5s and 10s) as ceilings, against leaderboard's worst real frame of
22ms. All four landed within 5s of a scroll-strip rebuild, and odds rebuilds
its strip 22 times in 45 minutes where leaderboard and stocks rebuild zero.
Both waits now start the work once and collect it on a later frame, with a
per-job backoff so work that keeps failing is retried on a timer rather than
respawned every frame. The caller draws the placeholder meanwhile: a
placeholder for a few frames beats a frozen panel for seconds, and unlike a
freeze it does not stall the other plugins.
Three consequences of no longer blocking, handled here:
- _create_ticker_image published self.ticker_image and *then* drew the
separator bars into it and rebuilt cached_array. Safe only while display()
waited for the rebuild. It now builds into a local and publishes last, so
the render thread never scrolls a strip missing its separators.
- The "attempting to..." warnings and the fallback's three info lines fired
once per rebuild when display() blocked. They would now fire once per frame
for the whole rebuild. Removed and demoted respectively.
- The live-data refresh deferred off the render thread only when
is_currently_scrolling() was true, which is False on the first frame of a
display cycle -- exactly when the update interval is most likely to have
elapsed. It now defers whenever the core supports it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 32 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Two problems the previous commit exposed rather than caused. First, display() re-requested the deferred refresh on every frame. The interval check compares against self.last_update, which is written only *inside* _perform_update. The work is handed to defer_update, and process_deferred_updates only runs while nothing is scrolling -- so for the whole 70s the odds marquee is on screen, the interval stayed elapsed and display() queued another ESPN refresh every frame. Observed firing every 8ms, against a deferred queue capped at 50 entries that drains five at a time onto the render thread. The request now stamps last_update when it is made, which is what the interval was always meant to measure. Second, not blocking on the strip rebuild would have regressed a bug this plugin already fixed once. Core clears the helper's cached_image/cached_array whenever the plugin reports an update -- roughly once a minute on a live rig -- and the old code rebuilt the strip inline while the panel waited. Simply not waiting would have shown "No odds data" over perfectly good games for the length of the rebuild, which is exactly the symptom the invalidation handling was written to stop. It does not need a rebuild. _perform_update calls _create_ticker_image before core invalidates, so ticker_image is already current when the cache is cleared; core cannot clear it because that attribute is private to the plugin. display() now re-seeds the helper from the strip in hand, using the array kept when it was built -- two attribute writes instead of a full recomposite, and the marquee never stops. Array before image: get_visible_portion reads cached_image.width and cached_array separately, and a new image against an old array is short by the difference, which reaches Image.frombytes as "not enough image data". The full rebuild is now only for having no strip at all -- cold start, or no games -- where display() returns every frame until it lands, which is also what keeps the render thread out of the helper while the worker publishes. Tests: test_display_defers_network.py now pins the stronger guard (deferral is correct whether or not the marquee is flagged as scrolling, since the flag is False on the first frame of a cycle), plus the interval stamp, the absence of any blocking get() in display(), and publish-after-cached_array ordering. test_scroll_cache_invalidation.py pins that invalidation re-seeds rather than recomposites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Zero rebuilds" is ambiguous on its own: it reads the same whether the re-seed absorbed every invalidation or the branch was never reached. A throttled heartbeat -- once a minute at most, since this is a per-frame path -- distinguishes the two, and tells an operator the cache is being invalidated without them having to turn on debug logging. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cleanup() cleared ticker_image and the helper's cache but not _ticker_array, which display() reads to decide whether it can re-seed. The stale array would have been handed to a helper whose image is None -- harmless today, because get_visible_portion checks cached_image first and the very next branch rebuilds, but it is a torn pair one reordering away from mattering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit stamped self.last_update when display() requested a
refresh, to stop the interval staying elapsed and re-queueing one every frame.
That cured the flood by breaking the refresh.
last_update is the field _perform_update tests on entry:
if current_time - self.last_update < current_interval:
return
The work is deferred, so it lands seconds later -- and finds last_update
freshly stamped, well inside the interval, and returns having done nothing.
The display-driven live refresh, which exists so scores and clocks move while
the marquee is on screen, would have been a no-op whenever the deferred call
came back within the interval. Data kept refreshing only because the plugin
scheduler calls update() on its own thread, independently; the display path
was quietly dead.
The request now has its own marker. _refresh_pending stops display() queueing
a second refresh while one is outstanding, and _refresh_requested_at ages it
out after max(interval, 300s) so a request core dropped -- it expires deferred
work on a 300s TTL and evicts it when the queue is full -- cannot wedge the
branch shut for good. last_update goes back to meaning "the data landed", so
_perform_update's own guard works as written.
_deferred_refresh clears the pending flag in a finally, so one failing refresh
does not stop display() ever asking again.
Verified on hardware before this change that the scheduler path is what keeps
the data current: 255 recomposites in 25 minutes against 3 interval requests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
display() now reads _ticker_array, _refresh_pending and _refresh_requested_at on every frame, and schedules through _deferred_refresh. _Ticker builds its own __init__ rather than running the plugin's, so it carried none of them and the first display() call raised AttributeError. Caught by CI, not locally: `pytest plugins/odds-ticker/` collects nothing from this file -- it exposes main(), not test_ functions -- so the local run was green while the file was never executed. scripts/run_plugin_tests.py, which is what CI invokes, runs each file standalone and does catch it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChuckBuilds
added a commit
that referenced
this pull request
Sep 8, 2026
#469 moved the strip rebuild onto a worker thread, which is right, but left _create_ticker_image writing the two helper fields image-first: self.scroll_helper.cached_image = strip self.scroll_helper.cached_array = strip_array That is the reverse of the order the same PR established as correct in display()'s re-seed, with a comment explaining why: _get_visible_portion_integer reads cached_image.width and cached_array as two separate statements, so a reader between the two assignments sees the new (wider) image against the old (shorter) array. It was safe while _create_ticker_image ran with display() blocked on a queue. It is not now: the rebuild runs on a worker, and the re-seed keeps display() scrolling the previous strip while it runs, so the render thread reads in exactly that window. Reproduced against the real helper: image=6392 array=6312, scrolled near the end ValueError: could not broadcast input array from shape (64,12,3) into shape (64,92,3) image=6392 array=6392, same position ok, (128, 64) Two adjacent statements, which is why 30 minutes of soak on #469 saw none of it. The existing test pins ticker_image relative to cached_array and says nothing about the two cache fields relative to each other, so it passed throughout. test_cached_array_is_written_before_cached_image walks both _create_ticker_image and display() in source order and requires every cached_image write to be preceded by a cached_array write. It fails on the pre-fix code naming the exact line. Manifest 1.4.3 -> 1.4.4. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The symptom
The odds marquee stuttered visibly where the other scrolling plugins did not — and its frame statistics said it shouldn't. Over 45 minutes on hardware, odds-ticker, leaderboard, stocks and news all sat at a 10.00 ms median with 0.08–0.16% stalls, and all four resolve to the same pacing (
100.0 px/s — 1px every 1 refresh = 100.0 fps, smooth).The medians hid it. The maxima did not:
A handful of multi-second frames barely move a stall percentage, but they are exactly what the eye catches.
The cause
display()testscurrent_time - self.last_update >= current_intervalto decide whether to refresh live game data.last_updateis written only inside_perform_update, so the test kept passing: 6,661 firings in 80 minutes, essentially one per frame.Most of those cost nothing —
_perform_updatehas its own early return — beyond one journald write per frame. But when the interval genuinely elapsed, one of them ran the full ESPN fetch plus a_create_ticker_imagerecomposite on the render thread.display()runs on the shared display loop, so that freezes the whole rotation, not just this plugin. 495Total width calculationlines in that window confirm ~22 recomposites, and the 1–5 s frames line up with them.The fix
is_currently_scrolling(), which is False on the first frame of a display cycle — exactly when the interval is most likely to have elapsed — so the blocking round trip could still reach the render thread.Two further blocking paths in
display()are removed as well. Both were written as a thread immediately followed by a blockingget():which is a blocking call wearing a thread as a disguise — the thread bought nothing, and could not have helped anyway since the work is
requestsand PIL. In fairness these were dead code on the test rig (0 trips in 80 minutes, at WARNING level, in a window carrying 79 other warnings). They are fixed because they are reachable on a Vegas rig and when a fetch returns no games, not because they caused this. They now start the work once and collect it on a later frame, with a per-job backoff.Three consequences of no longer blocking, handled here:
_create_ticker_imageboundself.ticker_imageand then drew the separator bars into it and rebuiltcached_array. It now builds into a local and publishes last._perform_updatecalls_create_ticker_imagebefore the invalidation lands, soticker_imageis already current and core cannot clear it.display()now re-seeds the helper from the strip in hand — two attribute writes, and the marquee never stops. Array before image:_get_visible_portion_integerreadscached_image.widthandcached_arrayseparately, and a new image against an old array is short by the difference, which reachesImage.frombytesas "not enough image data".display()blocked; they would now fire once per frame. Removed and demoted.Verification
30 minutes on hardware (
hdpi, 2×128×64 HUB75), rotation-gap windows excluded so only in-scroll frames are compared:Worst in-scroll frame 4829 ms → 19.98 ms; interval check 6,661 → 2 firings; zero fallback frames, zero tracebacks, zero
not enough image data.Tests:
test_display_defers_network.pypins the stronger deferral guard, the interval stamp, the absence of any blockingget()indisplay(), and publish-after-cached_arrayordering.test_scroll_cache_invalidation.pypins that invalidation re-seeds rather than recomposites. 7 passing.Manifest bumped 1.4.2 → 1.4.3 and
plugins.jsonregenerated.🤖 Generated with Claude Code