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

Automatically retry failed asset loads #17378

Open
alice-i-cecile opened this issue Jan 15, 2025 · 3 comments · May be fixed by #11349
Open

Automatically retry failed asset loads #17378

alice-i-cecile opened this issue Jan 15, 2025 · 3 comments · May be fixed by #11349
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 15, 2025

Do we not retry failed asset loads? I would assume they would fail, back off, and then retry.

We currently do not.

Originally posted by @alice-i-cecile in #17377 (comment)

@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 S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jan 15, 2025
@alice-i-cecile
Copy link
Member Author

This should be configurable!

@cart
Copy link
Member

cart commented Jan 15, 2025

As a note, this should be implemented in such a way that only retries things that aren't actual logic errors. Ex: we should not retry when a file is missing.

Unfortunately each class of error / context might also want different retry counts / delays / etc. Ex: a timed out HTTP request may have different retry requirements than a "too many file descriptors" error.

@alice-i-cecile alice-i-cecile added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jan 15, 2025
@brianreavis
Copy link
Contributor

brianreavis commented Jan 15, 2025

It's a little out of date with bevy/main, but I have it up-to-date on my fork and can update the PR if desired.

As a note, this should be implemented in such a way that only retries things that aren't actual logic errors. Ex: we should not retry when a file is missing. Unfortunately each class of error / context might also want different retry counts / delays / etc. Ex: a timed out HTTP request may have different retry requirements than a "too many file descriptors" error.

It does handle the above concerns.

// ...
            match read_error {
                AssetReaderError::NotFound(_) => {}
                AssetReaderError::Io(io_error) => match io_error.kind() {
                    ErrorKind::InvalidInput
                    | ErrorKind::InvalidData
                    | ErrorKind::NotFound
                    | ErrorKind::PermissionDenied
                    | ErrorKind::Unsupported => {}
                    _ => return self.retry_settings,
                },
                AssetReaderError::HttpError(status_code) => {
                    match status_code {
                        // Retry after server errors and rate limits
                        500..=599 | 429 => return self.retry_settings,
                        _ => {}
                    }
                }
// ...

@alice-i-cecile alice-i-cecile linked a pull request Jan 15, 2025 that will close this issue
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants