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

Asset Load Retry Plugin #11349

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brianreavis
Copy link
Contributor

@brianreavis brianreavis commented Jan 15, 2024

Objective

This PR adds a robust retry mechanism for retrying assets that fail to load, which is extremely useful if you have assets coming in over a network. It's configured for the WASM asset reader by default, but can easily support third-party asset readers (like bevy_web_asset).

Solution

The AssetLoadRetryPlugin is added to the DefaultPlugins, so it will work out-of-the box. To get retries to happen, you must use an AssetReader that provides get_retry_settings (like HttpWasmAssetReader), or provide your own source or asset-specific settings via the AssetLoadRetrier resource.

Customization Examples

Retry Settings By Source

let mut retrier = app.world.resource_mut::<AssetLoadRetrier>();
retrier.set_source_settings(
    AssetSourceId::Default,
    IoErrorRetrySettingsProvider::from(AssetLoadRetrySettings {
        max_attempts: 5,
        max_delay: Duration::from_millis(5000),
        starting_delay: Duration::from_millis(100),
        time_multiple: 2.0,
    }),
);

Retry Settings By Asset Type

let mut retrier = app.world.resource_mut::<AssetLoadRetrier>();
// Retry images with their own interval
retrier.set_asset_settings::<Image>(IoErrorRetrySettingsProvider::from(AssetLoadRetrySettings {
    max_attempts: 1,
    max_delay: Duration::from_millis(1000),
    starting_delay: Duration::from_millis(1000),
    time_multiple: 1.0,
}));

// Disable all retries for CoolText
retrier.set_asset_settings::<CoolText>(AssetLoadRetrySettings::no_retries());

Disable Retries for Certain Domains

let mut retrier = app.world.resource_mut::<AssetLoadRetrier>();

#[derive(Default)]
struct CustomRetrySettingsProvider(IoErrorRetrySettingsProvider);

impl ProvideAssetLoadRetrySettings for CustomRetrySettingsProvider {
    fn get_settings(
        &self,
        default_settings: AssetLoadRetrySettings,
        event: &UntypedAssetLoadFailedEvent,
    ) -> AssetLoadRetrySettings {
        // Ignore defaults from the source and let IoErrorRetrySettingsProvider
        // provide its preferred settings, given the error.
        let settings = self.0.get_retry_settings(default_settings, event);

        // Block any retries for bevyengine.org
        if settings.will_retry() {
            if event
                .path
                .to_string()
                .to_lowercase()
                .starts_with("https://bevyengine.org/")
            {
                return AssetLoadRetrySettings::no_retries();
            }
        }

        settings
    }
}

retrier.set_source_settings(
	AssetSourceId::Name("https".into()),
	CustomRetrySettingsProvider::default(),
);

Other Thoughts

  • I had built out a typed version of AssetLoadRetryPlugin<A> originally, but found the ergonomics to be... not good. Also had a few other drawbacks:
    • It felt confusing to have retry state spread out across asset types, when retry behavior is almost always source-specific.
    • Enabling it for all existing and future Asset types was near-impossible.
    • Supporting a global retry rate limit in the future (without introducing parallelism issues) would be difficult.
  • Probably outside this PR, but happy to address if desired: AssetReaderError::NotFound should probably be reconsidered/removed (?). It's convenient for AssetReaderError match expressions and easy to construct, but it overlaps:
    • AssetReaderError::Io(err) => { err.kind() == ErrorKind::NotFound }
    • AssetReaderError::Http(404)
    • We could drop NotFound and introduce is_not_found(&self) -> bool on the enum. Also fn io_not_found() -> Self to maintain easy construction perhaps, but that's kinda funky.
  • Retry jitter should be supported sometime.

Changelog

  • Added AssetLoadRetryPlugin which provides a robust retry mechanism for retrying assets that fail to load, which is extremely useful if you have assets coming in over a network. It's configured for the WASM asset reader by default, but can easily support third-party asset readers (like bevy_web_asset).

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds labels Jan 15, 2024
@alice-i-cecile
Copy link
Member

This retry plugin in this PR is more opinionated than the error events and can be split out if desired.

This feels like the right call. Looks super useful, but will require more thorough review (and more specialized expertise on the behalf of reviewers).

@brianreavis brianreavis changed the title Asset Error Events & Load Retry Plugin Asset Load Retry Plugin Jan 16, 2024
@brianreavis
Copy link
Contributor Author

This retry plugin in this PR is more opinionated than the error events and can be split out if desired.

This feels like the right call. Looks super useful, but will require more thorough review (and more specialized expertise on the behalf of reviewers).

👍 Done. I opened #11369 for the events and reworked this PR to be specifically about the retry plugin.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Jan 16, 2024
@alice-i-cecile
Copy link
Member

Temporarily blocked on #11369: that should be merged first :)

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Jan 17, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 17, 2024
# Objective

This adds events for assets that fail to load along with minor utility
methods to make them useful. This paves the way for users writing their
own error handling and retry systems, plus Bevy including robust retry
handling: #11349.

* Addresses #11288
* Needed for #11349

# Solution

```rust
/// An event emitted when a specific [`Asset`] fails to load.
#[derive(Event, Clone, Debug)]
pub struct AssetLoadFailedEvent<A: Asset> {
    pub id: AssetId<A>,
    /// The original handle returned when the asset load was requested.
    pub handle: Option<Handle<A>>,
    /// The asset path that was attempted.
    pub path: AssetPath<'static>,
    /// Why the asset failed to load.
    pub error: AssetLoadError,
}
```

I started implementing `AssetEvent::Failed` like suggested in #11288,
but decided it was better as its own type because:

* I think it makes sense for `AssetEvent` to only refer to assets that
actually exist.
* In order to return `AssetLoadError` in the event (which is useful
information for error handlers that might attempt a retry) we would have
to remove `Copy` from `AssetEvent`.
* There are numerous places in the render app that match against
`AssetEvent`, and I don't think it's worth introducing extra noise about
assets that don't exist.

I also introduced `UntypedAssetLoadErrorEvent`, which is very useful in
places that need to support type flexibility, like an Asset-agnostic
retry plugin.

# Changelog

* **Added:** `AssetLoadFailedEvent<A>`
* **Added**: `UntypedAssetLoadFailedEvent`
* **Added:** `AssetReaderError::Http` for status code information on
HTTP errors. Before this, status codes were only available by parsing
the error message of generic `Io` errors.
* **Added:** `asset_server.get_path_id(path)`. This method simply gets
the asset id for the path. Without this, one was left using
`get_path_handle(path)`, which has the overhead of returning a strong
handle.
* **Fixed**: Made `AssetServer` loads return the same handle for assets
that already exist in a failed state. Now, when you attempt a `load`
that's in a `LoadState::Failed` state, it'll re-use the original asset
id. The advantage of this is that any dependent assets created using the
original handle will "unbreak" if a retry succeeds.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@brianreavis
Copy link
Contributor Author

One topic for review is the future of AssetReaderError as outlined here by @thepackett (which is worth considering prior to merging). If allowing asset readers to define their own error types (like AssetLoader/AssetSaver) is desired, this PR will need to change slightly.

@Andrewp2
Copy link

One suggestion I have is that it would be nice if we could have some configurable random jitter added (see https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/), but otherwise LGTM.

@thepackett
Copy link
Contributor

One topic for review is the future of AssetReaderError as outlined here by @thepackett (which is worth considering prior to merging). If allowing asset readers to define their own error types (like AssetLoader/AssetSaver) is desired, this PR will need to change slightly.

To be honest, while I feel like adding error types to AssetReaders and AssetWriters would be good for flexibility, it is not urgent, and it's not something that I'm working on yet. For those reasons, I don't think we should wait to get this PR merged. Speaking of which, is this PR ready for reviews? If so, I can review it.

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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants