Skip to content

Commit b403c71

Browse files
sspickleclaude
andcommitted
fix(worker): the await is right, the reason given for it was wrong
Review finding 1 (correctness of the JUSTIFICATION, not of the code) plus finding 3. No behaviour change: comments, a docstring, an assert message, and one added assertion. e086450 said an unconditional await was needed because a rate() that stops yielding "cannot be stopped". That is false on this host. Stop is worker.terminate() on the page side — unconditional, needing nothing from the worker thread — and trinket's worker-client.js says so in its opening comment. A wrong reason attached to a right decision is a liability: the next reader checks it, finds Stop works fine without the await, and deletes the await. The true reason is narrower and worse. In a worker the host delivers browser events by CALLING __trinket_vpython_dispatch, and that call only gets a turn when the running coroutine gives one up. A rate() that returns without awaiting therefore dispatches no events and flushes no updates: the scene FREEZES while the program runs flat out. Stop surviving that is what makes it subtle rather than mild — a frozen picture from a program that is still running reads to a student as "vpython is broken", not as a hang, and Stop clearing it up is consistent with both stories. Corrected in all three places it appeared here; the report's copy goes with the trinket commit. Finding 3: test_rate_subtracts_user_code_time_from_the_period asserted only an upper bound, so "sleep zero whenever the body did any work" — a plausible mis-implementation of exactly this fix, and a much worse one than the flat sleep it replaced — would have passed it. It now also asserts elapsed > ideal * 0.7: compensating for the body's cost must not become free-running. Tests: 31 passed (unchanged count; the new bound rides on the existing case). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7y9rTA1r8r8EhEnPSQenR
1 parent e086450 commit b403c71

2 files changed

Lines changed: 30 additions & 7 deletions

File tree

‎tests/test_trinket_worker.py‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -183,13 +183,23 @@ async def loop():
183183
'rate(%d) with %.0f ms of user code took %.3f s for %d iterations; '
184184
'flat-sleep behaviour is ~%.3f s, compensated is ~%.3f s'
185185
% (target, work * 1000, elapsed, iterations, flat, ideal))
186+
# The other side of it, or "compensation" that just stopped sleeping whenever
187+
# the body did any work at all would pass the assertion above. Subtracting
188+
# the body's cost must not turn rate(N) into a free-running loop.
189+
assert elapsed > ideal * 0.7, (
190+
'rate(%d) with user code ran at %.1f Hz — it stopped pacing rather than '
191+
'compensating' % (target, iterations / elapsed))
186192

187193

188194
def test_rate_still_yields_when_user_code_overruns_the_period(worker_env):
189195
"""Safety: over-period work must not turn rate() into a non-yielding return.
190196
191-
If it stops yielding, the worker's event loop never runs — no scene events
192-
get dispatched, and the Stop button dies. That matters more than the pacing.
197+
In a worker the host delivers browser events by CALLING the transport's
198+
dispatch, which only gets a turn when the running coroutine gives one up. A
199+
rate() that returns without awaiting therefore freezes the scene — no events
200+
dispatched, no flush — while the program runs flat out. (Stop survives it:
201+
that is worker.terminate() on the page side and needs nothing from this
202+
thread. Which makes the symptom subtler, not milder.)
193203
"""
194204
vp, _ = worker_env
195205
calls = 5
@@ -214,7 +224,9 @@ async def loop():
214224
gained = asyncio.run(loop())
215225
assert gained >= calls, (
216226
'rate() yielded %d times in %d over-period calls — a loop that stops '
217-
'yielding starves the worker (no events, no Stop)' % (gained, calls))
227+
'yielding starves the event loop, so no browser event is dispatched and '
228+
'no update is flushed: the scene freezes while the program runs on'
229+
% (gained, calls))
218230

219231

220232
def test_rate_does_not_accumulate_debt_after_an_overrun(worker_env):

‎vpython/trinket_worker.py‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -128,10 +128,21 @@ async def _async_rate(maxRate):
128128
# flush we just issued onto the page without an added wait.
129129
remaining = 0.0 if last is None else (last + period) - time.monotonic()
130130
# Always await, even at zero. When user code overruns the period the
131-
# remainder is negative and there is nothing to wait for — but a bare
132-
# return would never yield to the event loop, and a worker whose loop
133-
# never yields dispatches no scene events and cannot be stopped.
134-
# asyncio.sleep(0) yields; clamping at 0 also means a loop that falls
131+
# remainder is negative and there is nothing left to wait for — but a
132+
# bare return would never yield to the event loop, and in a worker run
133+
# nothing else does either: the host delivers browser events by CALLING
134+
# __trinket_vpython_dispatch, and that call only gets a turn when the
135+
# running coroutine gives one up. A rate() that stops yielding therefore
136+
# freezes the scene — no events dispatched, no flush, a program running
137+
# flat out with a picture that never changes.
138+
#
139+
# It does NOT break Stop: Stop is worker.terminate() on the page side,
140+
# which is unconditional and needs no cooperation from this thread
141+
# (trinket's worker-client.js says so at the top). That makes the
142+
# symptom subtler, not milder — a frozen animation from a program that
143+
# is still running reads as "vpython is broken" rather than as a hang.
144+
#
145+
# asyncio.sleep(0) yields. Clamping at 0 also means a loop that falls
135146
# behind simply stays behind rather than banking debt and then bursting
136147
# through a batch of zero-sleep iterations to catch up.
137148
await asyncio.sleep(remaining if remaining > 0.0 else 0.0)

0 commit comments

Comments
 (0)