Skip to content

Commit

Permalink
⚡️ Faster scheduling of scheduleSequence (#5614)
Browse files Browse the repository at this point in the history
**Description**

<!-- Please provide a short description and potentially linked issues
justifying the need for this PR -->

With #3891, our target is to make scheduling faster. Scheduling is one
of the built-in arbitrary provided by fast-check. It aims to ease
detection of race-conditions by offering a simple way to detect such
issues in your code.

Up-to-now the code was doing a bit too many await/then letting it give
back the hand many times in a row while it could have been faster to
take it back. Additionally, this past behaviour (that we are trying to
fix with many PRs) used to be strange as awaiting 1 was ok, but 10-times
was not. More precisely there was a number of await working and passed
this number it stopped to work as usual and users started to need to use
other approaches (drop the `waitAll`).

This set of PRs not only addresses a potential perf concern but also
clarify what should work so that it appears more determistic for the
final users.

<!-- * Your PR is fixing a bug or regression? Check for existing issues
related to this bug and link them -->
<!-- * Your PR is adding a new feature? Make sure there is a related
issue or discussion attached to it -->

<!-- You can provide any additional context to help into understanding
what's this PR is attempting to solve: reproduction of a bug, code
snippets... -->

**Checklist** — _Don't delete this checklist and make sure you do the
following before opening the PR_

- [x] The name of my PR follows [gitmoji](https://gitmoji.dev/)
specification
- [x] My PR references one of several related issues (if any)
- [x] New features or breaking changes must come with an associated
Issue or Discussion
- [x] My PR does not add any new dependency without an associated Issue
or Discussion
- [x] My PR includes bumps details, please run `yarn bump` and flag the
impacts properly
- [x] My PR adds relevant tests and they would have failed without my PR
(when applicable)

<!-- More about contributing at
https://github.com/dubzzz/fast-check/blob/main/CONTRIBUTING.md -->

**Advanced**

<!-- How to fill the advanced section is detailed below! -->

**👀 Potentially risky** (risk: low): Our scheduler implementation was
not as fast as it was supposed to be. As such it accepted some
intermediate frames to be issued in-between two executions possibly
making some behavior looking buggy when users started to happen yet an
extra awaiting thing in the loop.

<!-- [Category] Please use one of the categories below, it will help us
into better understanding the urgency of the PR -->
<!-- * ✨ Introduce new features -->
<!-- * 📝 Add or update documentation -->
<!-- * ✅ Add or update tests -->
<!-- * 🐛 Fix a bug -->
<!-- * 🏷️ Add or update types -->
<!-- * ⚡️ Improve performance -->
<!-- * _Other(s):_ ... -->

<!-- [Impacts] Please provide a comma separated list of the potential
impacts that might be introduced by this change -->
<!-- * Generated values: Can your change impact any of the existing
generators in terms of generated values, if so which ones? when? -->
<!-- * Shrink values: Can your change impact any of the existing
generators in terms of shrink values, if so which ones? when? -->
<!-- * Performance: Can it require some typings changes on user side?
Please give more details -->
<!-- * Typings: Is there a potential performance impact? In which cases?
-->
  • Loading branch information
dubzzz authored Jan 14, 2025
1 parent b24964a commit 440e67f
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 26 deletions.
5 changes: 5 additions & 0 deletions .changeset/dirty-crabs-drop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"fast-check": major
---

⚡️ Faster scheduling of `scheduleSequence` (#4717)
5 changes: 5 additions & 0 deletions .changeset/lucky-gorillas-pretend.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"fast-check": major
---

⚡️ Faster scheduling of `scheduleSequence` (#4717)
Original file line number Diff line number Diff line change
Expand Up @@ -156,27 +156,30 @@ export class SchedulerImplem<TMetaData> implements Scheduler<TMetaData> {
// Placeholder resolver, immediately replaced by the one retrieved in `new Promise`
// eslint-disable-next-line @typescript-eslint/no-empty-function
let resolveSequenceTask = () => {};
const sequenceTask = new Promise<void>((resolve) => (resolveSequenceTask = resolve));
const sequenceTask = new Promise<{ done: boolean; faulty: boolean }>((resolve) => {
resolveSequenceTask = () => resolve({ done: status.done, faulty: status.faulty });
});

const onFaultyItemNoThrow = () => {
status.faulty = true;
resolveSequenceTask();
};
const onFaultyItem = (error: unknown) => {
onFaultyItemNoThrow(); // Faulty must resolve sequence as soon as possible without any extra then
throw error; // Faulty must stay faulty to avoid calling next items
};
const onDone = () => {
status.done = true;
resolveSequenceTask();
};

let previouslyScheduled = dummyResolvedPromise;
for (const item of sequenceBuilders) {
const [builder, label, metadata] =
typeof item === 'function' ? [item, item.name, undefined] : [item.builder, item.label, item.metadata];
const onNextItem = () => {
// We schedule a successful promise that will trigger builder directly when triggered
const registerNextBuilder = (index: number, previous: PromiseLike<unknown>) => {
if (index >= sequenceBuilders.length) {
// All builders have been scheduled, we handle termination:
// if the last one succeeds then we are done, if it fails then the sequence should be marked as failed
previous.then(onDone, onFaultyItemNoThrow);
return;
}
previous.then(() => {
const item = sequenceBuilders[index];
const [builder, label, metadata] =
typeof item === 'function' ? [item, item.name, undefined] : [item.builder, item.label, item.metadata];
const scheduled = this.scheduleInternal(
'sequence',
label,
Expand All @@ -185,28 +188,19 @@ export class SchedulerImplem<TMetaData> implements Scheduler<TMetaData> {
customAct || defaultSchedulerAct,
() => builder(),
);
return scheduled;
};
// We will run current item if and only if the one preceeding it succeeds
// Otherwise, we mark the run as "failed" and ignore any subsequent item (including this one)
previouslyScheduled = previouslyScheduled.then(onNextItem, onFaultyItem);
}
// Once last item is done, the full sequence can be considered as successful
// If it failed (or any preceeding one failed), then the sequence should be marked as failed (already marked for others)
// We always handle Promise rejection of previouslyScheduled internally, in other words no error will bubble outside
previouslyScheduled.then(onDone, onFaultyItemNoThrow);
registerNextBuilder(index + 1, scheduled);
}, onFaultyItemNoThrow);
};

registerNextBuilder(0, dummyResolvedPromise);

// TODO Prefer getter instead of sharing the variable itself
// Would need to stop supporting <es5
// return {
// get done() { return status.done },
// get faulty() { return status.faulty }
// };
return Object.assign(status, {
task: Promise.resolve(sequenceTask).then(() => {
return { done: status.done, faulty: status.faulty };
}),
});
return Object.assign(status, { task: sequenceTask });
}

count(): number {
Expand Down
38 changes: 38 additions & 0 deletions website/docs/migration/from-3.x-to-4.x.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,44 @@ If you prefer the previous behavior, you can disable this feature in version 4 b

Related pull requests: [#5590](https://github.com/dubzzz/fast-check/pull/5590)

### Faster `scheduler`

Since version 1.20.0, fast-check has included a primitive designed to help detect race conditions. This feature unlocked many advanced use cases and elevated the library's capabilities.

However, the previous implementation was slower than intended and allowed intermediate tasks to be created and executed between two scheduled ones. This inconsistency could lead to scenarios where code passed tests but later failed when additional microtasks were introduced. To address this, we have reworked the scheduler in version 4 to be faster, more consistent, and safer.

Consider the following example, where a scheduler instance `s` is used:

```ts
// `s`: an instance of scheduler provided by fast-check
s.schedule(Promise.resolve(1)).then(async () => {
await 'something already resolved';
s.schedule(Promise.resolve(2));
});
await s.waitAll();
```

In version 3, all scheduled tasks, including `Promise.resolve(2)`, would have been executed by the end of `s.waitAll()`. In version 4, however, `Promise.resolve(2)` remains pending. This is because during the `waitAll` loop, the scheduler processes `Promise.resolve(1)` and continues execution until `await 'something already resolved'`. At that point, the scheduler resumes its waiting sequence, but `Promise.resolve(2)` has not yet been scheduled and remains unknown. As a result, `waitAll` finishes before executing it.

This behavior makes the scheduler more predictable and prevents subtle issues. In contrast, version 3 behaved inconsistently when processing many immediately resolved tasks, as shown below:

```ts
// `s`: an instance of scheduler provided by fast-check
s.schedule(Promise.resolve(1)).then(async () => {
await 'something already resolved';
await 'something already resolved';
await 'something already resolved';
await 'something already resolved';
await 'something already resolved';
s.schedule(Promise.resolve(2));
});
await s.waitAll();
```

On this second example version 3 would have behaved as version 4 with `Promise.resolve(2)` still pending. The only difference between the two examples being the number of `await` before the next scheduled tasks. This improvement ensures unexpected behaviors in such edge cases and ensures consistent behavior.

Related pull requests: [#5600](https://github.com/dubzzz/fast-check/pull/5600), [#5604](https://github.com/dubzzz/fast-check/pull/5604), [#5614](https://github.com/dubzzz/fast-check/pull/5614)

## Advanced usages

### Custom reporters
Expand Down

0 comments on commit 440e67f

Please sign in to comment.