bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Asset Load Retry Plugin

Open brianreavis opened this issue 2 years ago • 6 comments

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).

  • Built atop #11369

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).

brianreavis avatar Jan 15 '24 03:01 brianreavis

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).

alice-i-cecile avatar Jan 15 '24 03:01 alice-i-cecile

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.

brianreavis avatar Jan 16 '24 10:01 brianreavis

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

alice-i-cecile avatar Jan 16 '24 14:01 alice-i-cecile

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.

brianreavis avatar Jan 17 '24 21:01 brianreavis

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.

Andrewp2 avatar Jan 18 '24 19:01 Andrewp2

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.

thepackett avatar Jan 18 '24 22:01 thepackett