Skip to content

Make GatedReader a test-only symbol, and allow all bevy_asset tests to all run single threaded. #18473

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

andriyDev
Copy link
Contributor

@andriyDev andriyDev commented Mar 22, 2025

Objective

  • Some tests have been disabled in single-threaded mode.
  • This was because the channel blocks the executing thread when in the asset reader. But that means we never get to the test line to open the gate! So we need multithreading just to let the reader block and the test can make progress in the background.

Solution

  • The solution to allowing all tests to run single threaded is to make GatedReader await for gates to open rather than blocking on it.
  • This however requires adding async-channel (already in tree). To avoid this additional dependency in most cases, I made this a dev-dependency.
  • However, this now means that GatedReader can only be used in tests (in bevy_asset). We could make this a feature flag, but it seems highly unlikely that real users need a GatedReader - and if they need it for their test, they can just fork the GatedReader.

Testing

  • The bevy_asset tests pass without setting the --features multi_threaded flag!

Migration Guide

bevy_asset::io::gated::GatedReader and bevy_asset::io::gated::GatedOpener are no longer accessible to users.

@andriyDev andriyDev marked this pull request as ready for review March 22, 2025 01:54
@andriyDev andriyDev force-pushed the no-multithreaded-dep branch from c382019 to 154d50a Compare March 22, 2025 01:55
@andriyDev andriyDev changed the title Make GatedReader a test-only symbol, and allow all tests to run single threaded. Make GatedReader a test-only symbol, and allow all bevy_asset tests to all run single threaded. Mar 22, 2025
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Testing A change that impacts how we test Bevy or how users test their apps D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Async Deals with asynchronous abstractions labels Mar 23, 2025
@andriyDev andriyDev force-pushed the no-multithreaded-dep branch from 154d50a to 340c7f0 Compare July 2, 2025 20:35
@alice-i-cecile alice-i-cecile requested a review from mockersf July 2, 2025 20:35
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! async_channel is fully no_std anyway, so it's inclusion as a true dependency in the future wouldn't be an issue.

@alice-i-cecile alice-i-cecile added the X-Uncontroversial This work is generally agreed upon label Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Testing A change that impacts how we test Bevy or how users test their apps D-Async Deals with asynchronous abstractions D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants