Skip to content

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

Merged
alice-i-cecile merged 5 commits into
bevyengine:mainfrom
andriyDev:no-multithreaded-dep
Aug 15, 2025
Merged

Make GatedReader a test-only symbol, and allow all bevy_asset tests to all run single threaded.#18473
alice-i-cecile merged 5 commits into
bevyengine:mainfrom
andriyDev:no-multithreaded-dep

Conversation

@andriyDev
Copy link
Copy Markdown
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
Copy Markdown
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
@mockersf mockersf added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 15, 2025
@github-actions
Copy link
Copy Markdown
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@andriyDev andriyDev added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 15, 2025
@andriyDev andriyDev added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 15, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 15, 2025
Merged via the queue into bevyengine:main with commit a76e0a2 Aug 15, 2025
36 checks passed
@andriyDev andriyDev deleted the no-multithreaded-dep branch August 15, 2025 04:24
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 M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants