[SPIKE] waitersSettled(): await waiter completion instead of polling for it - #525
[SPIKE] waitersSettled(): await waiter completion instead of polling for it#525NullVoxPopuli-ai-agent wants to merge 1 commit into
Conversation
04f2fe5 to
6505627
Compare
NullVoxPopuli-ai-agent
changed the title
| * `endAsync` that ends it and dropped at the same time -- so this only | ||
| * ever holds operations still in flight. | ||
| */ | ||
| private pendingOperations = new Map<T, { promise: Promise<void>; resolve: () => void }>(); |
There was a problem hiding this comment.
this should be a Set containing only promises.
Then,
settled can still Promise.all(the set's values)
There was a problem hiding this comment.
Done — the collection now holds only promises. Each one carries its own resolver (promise.resolve = resolve at creation), so there are no {promise, resolve} wrapper objects and settled() reads promises directly.
One deviation I want to flag rather than hide: it's a Map<Token, ResolvablePromise> and not a Set, because endAsync(token) has to settle that operation, which needs a lookup key. values() is the set of promises, so settled() is Promise.all(this.pendingPromises.values()). A literal Set<Promise> would need a second token→promise structure alongside it. Happy to switch if you'd rather have the Set and eat the extra map.
| } | ||
|
|
||
| settled(): Promise<void> { | ||
| const operations = [...this.pendingOperations.values()].map((operation) => operation.promise); |
There was a problem hiding this comment.
never ever ever array splat
There was a problem hiding this comment.
Fixed. Promise.all takes any iterable, so it's now Promise.all(this.pendingPromises.values()) — the splat and the .map are both gone.
| } | ||
|
|
||
| settled(): Promise<void> { | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
why? this is pointless, no?
There was a problem hiding this comment.
You're right — removed. NoopTestWaiter never calls register(), so it's never in getWaiters() and waitersSettled() would never reach it. settled() is optional on the interface, so nothing needs the stub.
6505627 to
b2c6750
Compare
| } | ||
|
|
||
| settled(): Promise<void> { | ||
| return Promise.all(this.pendingPromises.values()).then(() => {}); |
There was a problem hiding this comment.
why did you add this then? its unneeded
There was a problem hiding this comment.
Removed. It was only there to make the signature Promise<void> instead of Promise<void[]> — a pointless allocation and extra microtask for a value nobody reads. settled() and waitersSettled() now return Promise<unknown> and hand back Promise.all(...) directly.
Waiters already know when their operations complete, but consumers can only find out by polling. beginAsync now creates a promise for the operation, endAsync resolves it and drops it (so what is held is only work in flight), and waitersSettled() composes what is currently tracked. Each promise carries its own resolver, so the collection holds nothing but promises and settled() hands them straight to Promise.all. Operations begun after the call are not included -- settling can start more work, so callers needing a true fixpoint re-check. A Waiter implemented directly against the interface may not implement the new optional settled(); waitersSettled() returns a never-resolving promise while such a waiter is pending rather than claiming quiet it cannot verify, so callers race it against their own fallback tick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b2c6750 to
94bd234
Compare
Review follow-ups from emberjs/ember-test-waiters#525: each pending operation's promise carries its own resolver, so the map holds nothing but promises and settled() passes them straight to Promise.all -- no array splat, no wrapper objects, no pointless .then. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Companion to emberjs/ember-test-helpers#1574, exploring the RFC 957 test story (context: emberjs/ember.js#21520). Draft for discussion.
The idea
Waiters already know the moment each operation completes, but consumers can only find out by polling
hasPendingWaiters()on a timer. So:beginAsynccreates a promise for that operation,endAsyncresolves it and drops it from the pending map — which therefore only ever holds work still in flight,waitersSettled()composes the promises of whatever is currently tracked,Waitergains an optionalsettled()so each waiter answers for its own operations.NoopTestWaiter(production) resolves immediately; nothing changes for non-test builds.Two honest limits, both reflected in the API
Operations begun after the call are not included. Settling can start more work, so
waitersSettled()is a snapshot, and a caller that needs a true fixpoint re-checks. (The companionsettled()loops.)A
Waiterimplemented directly against the interface may not implementsettled()— itswaitUntilis pull-only and nothing obliges it to announce anything. Rather than claiming a quiet it cannot verify,waitersSettled()returns a never-resolving promise while such a waiter is pending, so callers race it against their own fallback tick and the pull-only waiter still works.Does it actually do the work?
Measured, not assumed. Instrumenting the companion
settled()to record which side of the race won, in an app whose suite exercises workers, rendering, and compilation: 91 of 92 iterations decided by the promises, 1 by the fallback.That measurement also surfaced a sharp edge worth recording: the fallback tick must be slower than a frame. At 10ms it beat frame-paced render ticks to the race and decided 30 of 117 iterations, costing an extra loop pass each time; at 50ms, 1 of 92.
Validation
base-testsapp: 56/56.@ember/test-helperstest-app against a linked build of this branch: 553/553; against published@ember/test-waiters(nowaitersSettled, degraded to the fallback tick): 553/553.🤖 Generated with Claude Code