Fix throw error from load resource in managers for support retries logic #8060
Conversation
… thrown after logging. This includes updates to the PIXI spine, JSON, Model3D, font face observer, sound, and image managers to improve robustness and error reporting.
- Moved processWithPromisePool and processAndRetryIfNeededWithPromisePool to static methods for better organization. - Introduced PromiseError and PromisePoolOutput types for enhanced error management. - Updated calls to processAndRetryIfNeededWithPromisePool to use the new static method reference.
4ian
left a comment
4ian
left a comment
There was a problem hiding this comment.
Hello! Thank your for submitting this. Overall I think it's indeed much better to fix these loading methods by allowing them to throw and have the caller properly retry as intended. Good catch :)
I've added a few comments and questions.
|
|
||
| async loadResource(resourceName: string): Promise<void> { | ||
| await this.getOrLoad(resourceName); | ||
| try { |
There was a problem hiding this comment.
This one might be unnecessary unless you have an idea like making it very clear it can throw?
There was a problem hiding this comment.
You are right! it was excessive! Thanks
Fixed!
| if (sound.status >= 200 && sound.status < 300) { | ||
| resolve(undefined); | ||
| } else { | ||
| reject(`HTTP error! status: ${sound.status}`); |
There was a problem hiding this comment.
I suggest an exception that gives more context to ease debugging:
| reject(`HTTP error! status: ${sound.status}`); | |
| reject(`HTTP error while preloading audio file in cache. Status is ${sound.status}.`); |
| } | ||
| ); | ||
| if (!response.ok) { | ||
| throw new Error(`HTTP error! status: ${response.status}`); |
There was a problem hiding this comment.
| throw new Error(`HTTP error! status: ${response.status}`); | |
| throw new Error(`HTTP error while loading bitmap font. Status is ${sound.status}.`); |
| } | ||
| await this._loadTexture(resource); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Same comment here: this looks unnecessary?
| } | ||
| } catch (error) { | ||
| logFileLoadingError(resource.file, error); | ||
| PIXI.Texture.removeFromCache(resourceUrl); |
There was a problem hiding this comment.
Have you observed this to be useful? I wonder if this will work because the internal texture id used by PixiJS could be different than the resourceUrl?
There was a problem hiding this comment.
Yes, I’ve checked that. This is needed so that PIXI.Texture.from performs a new network request and cancels caching of the previous result. I think in this context the URL is the unique identifier of the resource.
- Moved maxForegroundConcurrency, maxBackgroundConcurrency, and maxAttempt to static properties of ResourceLoader for better organization and accessibility. - Updated references throughout the ResourceLoader class to use the new static properties.
- Removed unnecessary try-catch blocks in the PIXI spine atlas and image managers, allowing errors to propagate naturally. - Enhanced error messages in the sound and bitmap font managers for clearer debugging information when HTTP errors occur.
2 participants
The main goal of this PR is to propagate exceptions from
loadResourcesback to the resource managers when a loading error occurs, so that theprocessAndRetryIfNeededWithPromisePoolretry logic can be applied.Typically, resources are downloaded from a CDN, and sometimes, for various reasons, a request for a resource may fail. In such cases, it’s very useful to have retry logic for downloads — and luckily, you already have it implemented. However, the resource managers did not propagate the thrown errors, which is required for
processAndRetryIfNeededWithPromisePoolto work correctly.In addition, I took the liberty of making
processAndRetryIfNeededWithPromisePoolandprocessWithPromisePoolstatic methods of theResourceLoaderclass. This makes it possible to reuse these functions in client code without having to duplicate them.I’d be grateful for a review and any feedback.
Best regards,
Viktor