Skip to content
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

Flaky doc test in bevy_tasks #6306

Closed
hymm opened this issue Oct 19, 2022 · 1 comment
Closed

Flaky doc test in bevy_tasks #6306

hymm opened this issue Oct 19, 2022 · 1 comment
Labels
A-Tasks Tools for parallel and async work C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Testing A change that impacts how we test Bevy or how users test their apps

Comments

@hymm
Copy link
Contributor

hymm commented Oct 19, 2022

Bevy version

Test was added in #4466

What you did

Reported by targrub in discord https://discord.com/channels/691052431525675048/692572690833473578/1032353522475348018.

Test here (https://github.com/bevyengine/bevy/blob/main/crates/bevy_tasks/src/task_pool.rs#L148-L173) fails in about 1 in 20 runs of cargo run -p bevy_tasks

What went wrong

I wrote the doc test thinking that it was impossible for the task that spawns another task to be inserted into the scope's task queue after the task that it spawns. That was an incorrect assumption.

    pub fn spawn<Fut: Future<Output = T> + 'scope + Send>(&self, f: Fut) {
        let task = self.executor.spawn(f);
        // ConcurrentQueue only errors when closed or full, but we never
        // close and use an unbouded queue, so it is safe to unwrap
        self.spawned.push(task).unwrap();
    }

The second task can be spawned between spawning the first task on the executor and inserting the tasks into the spawned queue.

This isn't really a problem as the docs state that a task spawned inside another task has non deterministic ordering. The fix should probably to just remove the assert and make it more clear when the results are deterministic and when it isn't.

@hymm hymm added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Oct 19, 2022
@hymm
Copy link
Contributor Author

hymm commented Oct 19, 2022

I'll try and put together a PR for this soon.

@hymm hymm added A-Tasks Tools for parallel and async work C-Docs An addition or correction to our documentation C-Testing A change that impacts how we test Bevy or how users test their apps and removed S-Needs-Triage This issue needs to be labelled labels Oct 19, 2022
@bors bors bot closed this as completed in 48e9dc1 Oct 20, 2022
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
# Objective

Fixes bevyengine#6306

## Solution
 Change the failing assert and expand example to explain when ordering is deterministic or not.

Co-authored-by: Mike Hsu <mike.hsu@gmail.com>
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this issue Dec 17, 2022
# Objective

Fixes bevyengine#6306

## Solution
 Change the failing assert and expand example to explain when ordering is deterministic or not.

Co-authored-by: Mike Hsu <mike.hsu@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

Fixes bevyengine#6306

## Solution
 Change the failing assert and expand example to explain when ordering is deterministic or not.

Co-authored-by: Mike Hsu <mike.hsu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Tasks Tools for parallel and async work C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Testing A change that impacts how we test Bevy or how users test their apps
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant