-
-
Notifications
You must be signed in to change notification settings - Fork 35.8k
Loader: Add abort()
.
#31276
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
base: dev
Are you sure you want to change the base?
Loader: Add abort()
.
#31276
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is great change but I'm wondering if the usage is going to be a bit out of sync with out users typically are using LoadingManager. Right now LoadingManager is often used to track the loading of a number of items being loaded so the same loading manager would then be passed to all the loaders to track that data. However with this "abort" model this means you would not be able to abort just a single load. I don't think this should block the PR but maybe something to consider for future changes to LoadingManager.
In past projects I've created a custom version of LoadingManager that dispatched the on* functions as events and allowed for "parenting" so events could be bubbled through other loading managers and each subset can be tracked separately for caching, cancelling, etc. Something like so:
const appLoadingManager = new LoadingManager();
const modelLoadingManager = new LoadingManager();
// "appLoadingManager" will consider all loads pushed to "modelLoadingManager".
appLoadingManager.subscribeTo( modelLoadingManager );
edit I suppose it should also be noted that even if "abort" is called on LoadingManager it's still the case that onComplete
could be called by the loaders 🤔
src/core/EventDispatcher.js
Outdated
/** | ||
* Removes all event listeners for the given event type. | ||
* | ||
* @param {string} type - The type of event. | ||
*/ | ||
removeEventListeners( type ) { | ||
|
||
const listeners = this._listeners; | ||
|
||
if ( listeners === undefined ) return; | ||
|
||
delete this._listeners[ type ]; | ||
|
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@linbingquan Can we elaborate on some practical use cases for this function? It seems more like a workaround for applications not managing their own event registration well.
It should be noted that adding this function means that EventDispatcher is becoming out of sync with the API of the browsers event system, which I believe was the original intent of the class. This might be okay but perhaps deserves some consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the PR is not using removeEventListeners()
anymore, I have removed the addition to EventDispatcher
as well. If there is no compelling use case, it's indeed better to keep the API in sync with the browser event system.
I've tried different approaches in the last days and I don't think there is an option to implement in a way that covers all potential use cases without introducing more fundamental changes to |
I'm not fully confident of how to classify "abort". I understand the argument that it should be an "OK" result since it's a controlled operation but on the other hand "abort" means the loaded asset can't be used for its intended purpose which sounds more like an error. |
That sounds interesting. Deriving |
Yeah I understand. To be clear, I think this approach is good. I just mean to suggest some further enhancements to LoadingManager for the future.
Thinking about it more I feel the current behavior is fine. There will inevitably be cases where a Loader is already finished and then a user aborts. The browser's fetch( url, { signal } )
.then( res => {
// fetch does not throw an error or abort
controller.abort();
} ) |
src/loaders/LoadingManager.js
Outdated
/** | ||
* Whether loading requests can be aborted with {@link LoadingManager#abort} or not. | ||
* | ||
* @type {boolean} | ||
* @default false | ||
*/ | ||
this.enableAbortManagement = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the value of this? Why not always register for an abort command in the Loaders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think of a viewer application. If the app creates a loader instance every time an asset should be imported, the default loading manager will be polluted with callbacks. It seems more save to register abort handlers only with custom loading managers and if the app really wants that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. This is something I'd expect loaders to unregister once the abort signal use was past. What if the LoadingManager
provides an AbortSignal that is replaced when "abort" is called and then requests can use AbortSignal.any to listen to both:
class LoadingManager {
// ...
abort() {
this.abortController.abort();
this.abortController = new AbortSignal();
}
}
class FileLoader {
// ...
load( /* ... */ ) {
const req = new Request( url, {
headers: new Headers( this.requestHeader ),
credentials: this.withCredentials ? 'include' : 'same-origin',
signal: AbortSignal.any( this._abortController.signal, manager.abortController.signal ),
} );
// ...
}
}
This way "abort" will always work without the need for a separate flag, which I think would be expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this approach before I came up with the version of the PR. The major issue is that AbortController
is now hardwired in LoadingManager.abort()
which a problem for loaders which do not rely on fetch
(like ImageLoader
). We should head for a solution that makes no assumptions about how an abort operation in a loader is implemented. Hence, I vote for an event based mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no requirement that AbortSignal only be used with fetch or Request. An "abort" event can be registered like so:
const controller = new AbortController();
controller.signal.addEventListener( 'abort', () => console.log( 'ABORTED!' );
controller.abort();
// ABORTED!
The difference is that the browser's GC logic can now clean up event callbacks whose scope has been GC'd. I think this can be used very broadly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to the new approach. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great to me! The only other consideration is that apparently AbortSignal.any
is a bit new (I thought it had been around since AbortSignal was added). But it's been supported in all major browsers since March 2024. I'm not sure what policy on this is but it won't break any build processes since it's not new syntax and should be able to be polyfilled by users if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, if the polyfill is compact we probably should embed it into the project. What concerns me is that Safari supports this feature only since 17.4 which is a quite recent release. If a platform does not support AbortSignal.any
, most loaders won't work which is a bummer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we simply do this:
signal: ( typeof AbortSignal.any === 'function' ) ? AbortSignal.any( [ this._abortController.signal, this.manager.abortController.signal ] ) : this._abortController.signal
And state aborting on manager level is only supported with latest browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented this fallback now along with minor doc updates. Should we give the PR in the current state a try?
Fixed #20705.
Closes #23070, #29634.
Description
Aborting loading requests is a feature that has been requested more than once. Previous PRs weren't ideal though so this one is an attempt to find a good compromise.
The idea is to introduce
Loader.abort()
as an abstract method that can be implemented by concrete loaders. The loader implementation itself must decide how an abort operation is implemented.FileLoader
andImageBitmapLoader
do this withAbortController
but other loaders might use different strategies.The PR also introduces
LoadingManager.abort()
that makes it easier to abort the loading process of more complex loaders likeGLTFLoader
. If you create an instance ofLoadingManager
, you can now setenableAbortManagement
totrue
which enables abort management on loading manager level. Loaders using this manager will listen to anabort
event that will trigger theirabort()
implementation.