-
-
Notifications
You must be signed in to change notification settings - Fork 255
[SPIKE] settled(): await settledness instead of polling for it #1574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
06251b8
ed7c7b8
39b0024
4da40d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,15 +1,55 @@ | ||||||||
| // @ts-ignore: this is private API. This import will work Ember 5.1+ since it | ||||||||
| // "provides" this public API, but does not for earlier versions. As a result, | ||||||||
| // this type will be `any`. | ||||||||
| import { _backburner } from '@ember/runloop'; | ||||||||
| import { | ||||||||
| macroCondition, | ||||||||
| dependencySatisfies, | ||||||||
| importSync, | ||||||||
| } from '@embroider/macros'; | ||||||||
| import { Test } from 'ember-testing'; | ||||||||
|
|
||||||||
| import { nextTick } from './-utils.ts'; | ||||||||
| import waitUntil from './wait-until.ts'; | ||||||||
| import { hasPendingTransitions } from './setup-application-context.ts'; | ||||||||
| import { hasPendingWaiters } from '@ember/test-waiters'; | ||||||||
| import { buildWaiter, hasPendingWaiters } from '@ember/test-waiters'; | ||||||||
| import * as testWaiters from '@ember/test-waiters'; | ||||||||
| import type DebugInfo from './-internal/debug-info.ts'; | ||||||||
| import { TestDebugInfo } from './-internal/debug-info.ts'; | ||||||||
| import renderSettled from './-internal/render-settled.ts'; | ||||||||
|
|
||||||||
| // This is private API. Runloop-less builds of ember-source (the RFC 957 | ||||||||
| // spikes) do not export `_backburner` at all, so it is read off the module | ||||||||
| // namespace -- a missing export degrades to `undefined` here instead of a | ||||||||
| // build-time missing-export error in consuming apps. | ||||||||
| const _backburner: any = (importSync('@ember/runloop') as any)._backburner; | ||||||||
|
|
||||||||
| // Ember builds that schedule rendering without the runloop report the | ||||||||
| // EDGES of rendering work (pending / complete) rather than exposing a | ||||||||
| // pollable flag. Bridging those edges into a test waiter folds rendering | ||||||||
| // into the same settledness protocol as every other async source: it | ||||||||
| // needs no clause of its own in `isSettled` below, and a render that | ||||||||
| // never completes is reported by name in test-waiter debug output. | ||||||||
| const renderWaiter = buildWaiter('@ember/test-helpers:render'); | ||||||||
| let renderWaiterToken: unknown = null; | ||||||||
|
|
||||||||
| const usesRenderWaiter = (() => { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why all of this condition? we don't care, right? because we can just await renderSettled()
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Same limber tutorial suite, runloop-less ember build, only difference is this block:
Reproducible, both directions, re-run to confirm. The reason the awaited promise isn't sufficient: So the bridge isn't a second settledness mechanism — it's how (I haven't traced the tutorial hang to the exact line inside its compile pipeline — what I can say confidently is the A/B above and that the missing sync signal is the only behavioural difference.) |
||||||||
| if (macroCondition(dependencySatisfies('ember-source', '>=4.5.0-beta.1'))) { | ||||||||
| const renderer = importSync('@ember/renderer') as any; | ||||||||
|
|
||||||||
| if (typeof renderer._onRenderSettledChange === 'function') { | ||||||||
| renderer._onRenderSettledChange((pending: boolean) => { | ||||||||
| if (pending) { | ||||||||
| renderWaiterToken ??= renderWaiter.beginAsync(); | ||||||||
| } else if (renderWaiterToken !== null) { | ||||||||
| const token = renderWaiterToken; | ||||||||
|
|
||||||||
| renderWaiterToken = null; | ||||||||
| renderWaiter.endAsync(token); | ||||||||
| } | ||||||||
| }); | ||||||||
|
|
||||||||
| return true; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| return false; | ||||||||
| })(); | ||||||||
|
|
||||||||
| let requests: XMLHttpRequest[]; | ||||||||
| const checkWaiters = Test.checkWaiters; | ||||||||
|
|
@@ -139,15 +179,17 @@ export interface SettledState { | |||||||
| @returns {Object} object with properties for each of the metrics used to determine settledness | ||||||||
| */ | ||||||||
| export function getSettledState(): SettledState { | ||||||||
| const hasPendingTimers = _backburner.hasTimers(); | ||||||||
| const hasRunLoop = Boolean(_backburner.currentInstance); | ||||||||
| const hasPendingTimers = _backburner ? _backburner.hasTimers() : false; | ||||||||
| const hasRunLoop = _backburner ? Boolean(_backburner.currentInstance) : false; | ||||||||
| const hasPendingLegacyWaiters = checkWaiters(); | ||||||||
| const hasPendingTestWaiters = hasPendingWaiters(); | ||||||||
| const pendingRequestCount = pendingRequests(); | ||||||||
| const hasPendingRequests = pendingRequestCount > 0; | ||||||||
| // TODO: Ideally we'd have a function in Ember itself that can synchronously identify whether | ||||||||
| // or not there are any pending render operations, but this will have to suffice for now | ||||||||
| const isRenderPending = !!hasRunLoop; | ||||||||
| // On runloop-driven builds, a pending render is observable as backburner's | ||||||||
| // autorun instance. Scheduler-driven builds report render edges into the | ||||||||
| // render waiter above, so a pending render is already counted in | ||||||||
| // `hasPendingTestWaiters` -- reporting it here too would double-count it. | ||||||||
| const isRenderPending = usesRenderWaiter ? false : !!hasRunLoop; | ||||||||
|
|
||||||||
| return { | ||||||||
| hasPendingTimers, | ||||||||
|
|
@@ -209,6 +251,91 @@ export function isSettled(): boolean { | |||||||
| @public | ||||||||
| @returns {Promise<void>} resolves when settled | ||||||||
| */ | ||||||||
| export default function settled(): Promise<void> { | ||||||||
| return waitUntil(isSettled, { timeout: Infinity }).then(() => {}); | ||||||||
| /** | ||||||||
| * Waiter completion is announced by `@ember/test-waiters` versions that | ||||||||
| * export `waitersSettled`; older ones are pull-only, and the fallback | ||||||||
| * tick below drives the loop instead. | ||||||||
| * | ||||||||
| * @private | ||||||||
| */ | ||||||||
| const maybeWaitersSettled = ( | ||||||||
| testWaiters as unknown as { waitersSettled?: () => Promise<unknown> } | ||||||||
| ).waitersSettled; | ||||||||
|
|
||||||||
| const waitersSettled: (() => Promise<unknown>) | null = | ||||||||
| typeof maybeWaitersSettled === 'function' ? maybeWaitersSettled : null; | ||||||||
|
|
||||||||
| /** | ||||||||
| * How long the fallback tick waits. | ||||||||
| * | ||||||||
| * When waiters announce completion, this tick is a safety net for the | ||||||||
| * sources that cannot: `Waiter` implementations written against the | ||||||||
| * interface directly, legacy `Ember.Test.registerWaiter` callbacks, and | ||||||||
| * request counters. It must then comfortably exceed a frame, because a | ||||||||
| * render tick can be frame-paced -- at 10ms it beat rendering to the | ||||||||
| * race often enough to decide a quarter of all iterations (measured 30 | ||||||||
| * of 117), costing an extra pass each time; at 50ms it decided 1 of 92. | ||||||||
| * | ||||||||
| * Without waiter notification it is the loop's only clock, so it stays | ||||||||
| * at the cadence the previous `waitUntil`-based implementation used. | ||||||||
| * | ||||||||
| * @private | ||||||||
| */ | ||||||||
| const FALLBACK_MS = waitersSettled === null ? 10 : 50; | ||||||||
|
|
||||||||
| function fallbackTick(): Promise<void> { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete this function
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Function deleted and inlined — but the 50ms timer itself has to stay, and I have data rather than an opinion on that now. I first built exactly the minimal version ( The reason is that not everything announces completion: run loop timers ( With the timer inlined and the loop kept: 553/553. |
||||||||
| return new Promise((resolve) => setTimeout(resolve, FALLBACK_MS)); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Resolves when no waiter is pending, on versions that can tell us. | ||||||||
| * Otherwise never resolves, leaving the fallback tick to drive. | ||||||||
| * | ||||||||
| * @private | ||||||||
| */ | ||||||||
| function waitersQuiet(): Promise<unknown> { | ||||||||
| return waitersSettled === null ? new Promise(() => {}) : waitersSettled(); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Yields to the task queue, so quiet is observed from a macrotask. | ||||||||
| * | ||||||||
| * @private | ||||||||
| */ | ||||||||
| function macrotask(): Promise<void> { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete this function
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Function deleted and inlined too. The 0ms yield it did stays, for the same measured reason as before: quiet observed in microtask context beats already-queued task sources (worker messages, zero-delay timers) that are about to register waiters or dirty state, and |
||||||||
| return new Promise((resolve) => setTimeout(resolve, 0)); | ||||||||
| } | ||||||||
|
|
||||||||
| export default async function settled(): Promise<void> { | ||||||||
| // Settledness is awaited rather than polled: rendering resolves | ||||||||
| // `renderSettled()` when it completes, and waiters resolve | ||||||||
| // `waitersSettled()` from the operations' own completion promises. The | ||||||||
| // fallback tick is raced alongside them only to cover sources that | ||||||||
| // cannot announce completion -- when everything announces, it never | ||||||||
| // decides anything. | ||||||||
| // | ||||||||
| // Two properties this loop must preserve: | ||||||||
| // | ||||||||
| // 1. Quiet is confirmed FROM A MACROTASK. Task sources that are | ||||||||
| // already queued (worker messages, zero-delay timers) may register | ||||||||
| // waiters or dirty tracked state, and an observation made in | ||||||||
| // microtask context would win the race against them and settle | ||||||||
| // early. The previous waitUntil-based implementation imposed this | ||||||||
| // boundary implicitly by scheduling every check via setTimeout. | ||||||||
| // | ||||||||
| // 2. It re-checks. Completing the work that was pending can start | ||||||||
| // more of it, so one pass proves nothing; the loop runs until a | ||||||||
| // pass observes everything quiet. | ||||||||
| for (;;) { | ||||||||
| await Promise.race([ | ||||||||
| Promise.all([renderSettled(), waitersQuiet()]), | ||||||||
| fallbackTick(), | ||||||||
| ]); | ||||||||
|
|
||||||||
| await macrotask(); | ||||||||
|
|
||||||||
| if (isSettled()) { | ||||||||
| return; | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad bot. this is not what I meant.
I meant in the test-waiters package, when we go beginAsync, we can have that be internally new Promise, and then endAsync resolves the promise.
test-waiters can have a waitersSettled() function which sees if all known promises are resolved (tho, if they resolve, I would also assume they remove themselves from the structure that waitersSettled() would look at)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, and you're right — I built the aggregate version (one module-level deferred that resolved when
hasPendingWaiters()went false), not per-operation promises. Redone as you described, in emberjs/ember-test-waiters#525:beginAsynccreates a promise for that operationendAsyncresolves it and deletes it from the pending map, so the map only ever holds work in flightwaitersSettled()composes the promises of what's currently tracked, via a new optionalsettled()on each waiterYour design is also strictly better than mine for a reason I hadn't considered: my version called
hasPendingWaiters()on everyendAsync, which walks every registered waiter and buildsdebugInfoarrays for the pending ones. Per-operation promises do no scanning at all —endAsyncresolves one deferred.One case the interface forced a decision on: a
Waiterimplemented directly against the interface has nosettled()to compose. Rather than reporting a quiet it can't verify,waitersSettled()returns a never-resolving promise while such a waiter is pending, so the caller's fallback tick drives and pull-only waiters keep working.Re-validated: test-waiters
base-tests56/56; this repo's test-app 553/553 both against a linked build of the new API and against published test-waiters (degraded to the fallback); limber's full chrome matrix green with 7/7 repeat runs against the runloop-less spike.