Ability to cancel ongoing HTTP requests in loaders

[mistic100]

Related issue: #20705

Description

This adds an optional abortSignal to all loaders using FileLoader.

Usage :

const loader = new FileLoader();

const abortCtrl = new AbortController();

loader.setAbortSignal(abortCtrl.signal);

loader.load(url);

abortCtrl.abort();

---

This design was done as requested by gkjohnson. I personally would prefer to pass the abort signal to each load call, in order to be able to use the same loader for multiple parallel requests (see this reply) and thus no having the overhead of instanciating multiple loader whild loading a bunch of tiled textures for example.

[mrdoob]

Hmm, I wonder if there is a more elegant approach...

[gkjohnson]

Hmm, I wonder if there is a more elegant approach...

I think there is -- and I don't think passing an abort signal into Loader.load is necessarily a complete solution, either. GLTFLoader, for example, can call load on subsequent bin files or textures even when parse is called and those should be cancellable, as well.

With fetch now being used in FileLoader (#22510) I imagined Loader options eventually moving to an object that more directly reflected the fetch option interface which would naturally include the ability to set the abort signal:

const controller = new AbortController();
const loader = new GLTFLoader();
loader.fetchOptions = {
  headers: { ... },
  credentials: ...,
  mode: ...,
  signal: controller.signal
};

// or

loader.setFetchOptions( { ... } );

These options would be propagated through all subsequently created file loaders for the file so they would all be aborted simultaneously. Alternatively a Loader.setAbortSignal( controller.signal ) API could be provided.

Interface aside there are naturally some corner cases and race conditions to consider, as well. What happens if the abort signal is triggered while loader is doing some async processing and then a new fetch is fired? Does the subsequent fetch abort since the signal passed into the fetch had been triggered already? I'm not sure what the behavior is here and I think that should be understood. And this is probably for a more advanced implementation but in the future it might make sense for the abort signal to cancel any async processing that might happen if the signal is aborted after the file has downloaded but before a model has finished processing.

@mrdoob do you have a preference on API?

[mistic100]

Initially I wanted "FileLoader#load" to return a custom object wrapping the AbortController, usefull for future potential features. And similar to the previous version which returned the XMLHttpRequest object.

But this is not possible because when we hit the cache, the cached value is returned immediately (why though ?)

[mistic100]

@gkjohnson

Interface aside there are naturally some corner cases and race conditions to consider, as well. What happens if the abort signal is triggered while loader is doing some async processing and then a new fetch is fired? Does the subsequent fetch abort since the signal passed into the fetch had been triggered already? I'm not sure what the behavior is here and I think that should be understood. And this is probably for a more advanced implementation but in the future it might make sense for the abort signal to cancel any async processing that might happen if the signal is aborted after the file has downloaded but before a model has finished processing.

As far as I can see, not special handling was done when three.js used XMLHttpRequest that can already be aborted.

One difference though is that XMLHttpRequest errored with a Error with type=abort. Fetch errors with a DOMException with name=AbortError.

Also there is an issue when reusing an AbortController which was already triggered : https://jsfiddle.net/mistic100/fn9ba6ym/2/
you can comment the second ctrl.abort() but it will still abort the second request.
So we cannot have a single AbortController on the loader

[mistic100]

@mrdoob can you tell me how I can improve this ?

If you are willing to make a breaking change here https://github.com/mrdoob/three.js/blob/dev/src/loaders/FileLoader.js#L36 I have another idea which is to make "load" return a custom controller. It could be an event emitter with "load", "error", "progress" and "cancel" events. And a "cancel" method.

This way it ensures each call to "load" uses a new AbortController. It also prevents to add another extra parameter.

[gkjohnson]

As far as I can see, not special handling was done when three.js used XMLHttpRequest that can already be aborted.

As far as I know there was no method to abort a request in the previous FileLoader implementation using XMLHttpRequest.

Also there is an issue when reusing an AbortController which was already triggered : https://jsfiddle.net/mistic100/fn9ba6ym/2/ you can comment the second ctrl.abort() but it will still abort the second request. So we cannot have a single AbortController on the loader

There's nothing wrong with creating a new Loader per load if you want to use multiple abort controllers. Many loaders already designed with load options seem to be designed with this intent because once a load is started you cannot change the options without impacting the already triggered load. It also allows the same abort controller to be used in multiple places at once.

The way LoadingManager.getHandler works is a bit of a wrench, though, because it returns the same Loader instance every time meaning you can't change options.

[mistic100]

As far as I can see, not special handling was done when three.js used XMLHttpRequest that can already be aborted.

As far as I know there was no method to abort a request in the previous FileLoader implementation using XMLHttpRequest.

Yes there were : "load" returned the XMLHttpRequest (unless hitting the cache) which can be aborted. https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/abort

I used it in https://github.com/mistic100/Photo-Sphere-Viewer and it worked perfectly.

Also there is an issue when reusing an AbortController which was already triggered : https://jsfiddle.net/mistic100/fn9ba6ym/2/ you can comment the second ctrl.abort() but it will still abort the second request. So we cannot have a single AbortController on the loader

There's nothing wrong with creating a new Loader per load if you want to use multiple abort controllers. Many loaders already designed with load options seem to be designed with this intent because once a load is started you cannot change the options without impacting the already triggered load. It also allows the same abort controller to be used in multiple places at once.

I think there is a misunderstanding here : and AbortController object cannot be used twice, once it was triggered it stays in an aborted state and will immediately cancel any new "fetch" it is attached too. That's what my jsffidle tries to exhibit. Confirmed here https://stackoverflow.com/a/64795615/1207670

So making the AbortController a property of the loader will IMHO only confuse people.

I personnally am all for reusing object when they can be. Instanciate a new loader everytime seems overkill.

[gkjohnson]

I used it in https://github.com/mistic100/Photo-Sphere-Viewer and it worked perfectly.

I see. Thanks I didn't realize FileLoader returned the request. Either way it seems the the requests could not be aborted through the model loader interfaces -- only if FileLoader was used directly.

I personnally am all for reusing object when they can be. Instanciate a new loader everytime seems overkill.

I'm just suggesting that it's already a requirement in some cases and I think there's little benefit to reusing the loaders in most scenarios.

I think there is a misunderstanding here : and AbortController object cannot be used twice, once it was triggered it stays in an aborted state and will immediately cancel any new "fetch" it is attached too.

No I understand this. If an AbortController has already been aborted then it will abort any requests that try to use it after that. And this is the behavior you want so subsequent loads within a single loader are cancelled (ie GLTFLoader subsequently loading a texure). If the model load is aborted before it has started loading a texture it will prevent that load.

All I was saying was that you could use the same abort controller for the multiple requests as long as you want to abort both requests simultaneously:

const controller = new AbortController();
const signal = controller.signal;
fetch( url1, { signal } ).then( () =>{} );
fetch( url2, { signal } ).then( () =>{} );

controller.abort();

[mistic100]

Okay I understand your point. But is it really useful though ?

I have another proposal :

• make each Loader instance have it's own AbortController (not configurable)
• add a "Loader#abort()" method
• make "load" errors immediatley with a clear log message if the loader is already aborted

This way it is clear that the whole loader is aborted and cannot be reused. It also allows to make multiple request with the same loader and cancel them all at once.

Remains the question the loader manager handler, I personnally never used it.

[gkjohnson]

Okay I understand your point. But is it really useful though ?

I think it is useful to be able to reuse an AbortController, yes. If a user is loading a large scene with lots of assets and then switches scenes, cancelling the load then a single abort controller can be aborted. Of course this can be accomplished in other ways but this pattern is enabled and built into the AbortController and fetch APIs so it seems odd to preclude it.

make each Loader instance have it's own AbortController (not configurable)

The AbortController should be configurable. Again consider a GLTF model. They often first load the main GLTF json, then subsequently load a bin file and other textures. If the load is aborted after the JSON file has loaded but before the bin files and textures have finished then they need to be aborted, as well. The GLTFLoader could keep track of every loader it creates internally but that's a lot more technically complex than just using the AbortController as designed.

[mistic100]

@gkjohnson So I implemented your solution. Please tell me what you think before I try to update the examples loaders and the doc.

I also made an implementation on the ImageLoader by clearing "src"

[mistic100]

The other thing to note is that any loaders in the examples folder that load subsequent files (GLTFLoader, ColladaLoader, etc) will need to be updated to pass the abort controller through to the internally-created loaders but I expect that can happen in other PRs.

I have already prepared the change. I will commit it with the doc update in this same PR.

[mistic100]

I fail to understand why the E2E tests are failing on image comparaison...

[mistic100]

This is more or less finished, can it be reviewd ? thanks

[rhuitl]

I'm currently working on implementing request cancellation in an application that renders tiled raster maps, and I found it convenient to be able to cancel individual load() requests, instead of being forced to use a separate loader for each tile. I might have dozens of requests going at a time, and usually I'm trying to avoid creating new objects to minimize the pressure on the garbage collector. I'm not sure if creating new loaders for every load request is significant enough compared to the other garbage created by a load request, but I wanted to mention it here. Maybe there's also a hybrid approach where we can have an abort controller per loader and a parameter to the load() method to override it.

[segments-tobias]

Any updates on the review @gkjohnson ?

[mrdoob]

Is there a way of implementing this without having to modify all the loaders? 🤔

[gkjohnson]

Is there a way of implementing this without having to modify all the loaders? 🤔

I expect there'd have to be some kind of change to all loaders - particularly those that kick off loads of other files. Ie the GLTFLoader will create a file loader for the main content, then it will create a texture loader, etc. All of those loaders should be relying on the same abort signal and be cancelled if the original signal is aborted.

[mistic100]

Is there a way of implementing this without having to modify all the loaders? thinking

Not all loaders extends from Loader (or at least override the default behaviour by instanciating sub-loaders), so I don't see how this would be possible.

[speigg]

Also needing to be able to abort individual load() requests (not the entire loader).

[mistic100]

Also needing to be able to abort individual load() requests (not the entire loader).

@speigg this was the original design but I was asked to change. However as long as you don't load multiple files at the same time you will be able to reuse the loader :

const ctrl1 = new AbortController();

loader.setAbortSignal(ctrl1.signal);
loader.load(url1);

if (maybe) {
  ctrl1.abort();
}

// later
const ctrl2 = new AbortController();

loader.setAbortSignal(ctrl2.signal);
loader.load(url2);

But if you load multiples files at the same time, yes the signal will cancel all requests. I think it makes sense toward how THREE Loaders are designed, and even how the AbortController was designed to be shared accross multiple requests.

[trusktr]

loader.setAbortSignal(ctrl1.signal);
loader.load(url1);

That's not a good pattern, it will be error prone, people will accidentally leave aborted signals in place and will unintentionally do the wrong thing as @gkjohnson was mentioning.

The API should accept a signal per load operation, just like fetch, to match idiomatic patterns:

const ctrl1 = new AbortController();

loader.load(url1, ..., ctrl1.signal);

if (maybe) {
  ctrl1.abort();
}

// later
const ctrl2 = new AbortController();

loader.load(url2, ..., ctrl2.signal);

Each individual load action receiving a signal encourages better code, less error prone.

A more invasive approach (breaking change), but even less error prone, is to return an abort function (the abort controller being internally created) as well as the result:

const [texture, abort] = textureLoader.load(...)

if (maybe) abort()

or

const state = textureLoader.load(...)
// returns { texture, abort }

if (maybe) state.abort()

Perhaps the best and most flexible is allowing both options (so single abort signal can be shared, or each call can make one) but the second option is definitely a breaking API change.

[mistic100]

@trusktr that is what I did in the first interation of the PR but was asked to change it.

[mistic100]

Could this PR get some attention ?

[alessiocancian]

@mrdoob @gkjohnson any update about this?

[gkjohnson]

@\mrdoob @\gkjohnson @\trusktr any update about this?

I've given my opinions on this. I think other maintainers like @mrdoob or @Mugen87 need to give their opinion and an API should be settled on before more work is done.

[alessiocancian]

@\mrdoob @\gkjohnson @\trusktr any update about this?

I've given my opinions on this. I think other maintainers like @mrdoob or @Mugen87 need to give their opinion and an API should be settled on before more work is done.

If they don't reply I think this could progress following your API proposal, which seems fair to me

[mistic100]

The current state of the PR already takes gkjohnson reviews into account.

I will not take time to rebase until I am sure it will be merged relatively soon. But as soon as a maintainer gives its approval on the principle I will resolve the conflicts, polish it and update the doc.

[alessiocancian]

@mrdoob @Mugen87 could you please give your opinion so this can progress?

[alessiocancian]

Sorry for bothering again but I feel there are really some cases where the abort controller would benefit a lot. For example my problem is that I need to load large models from S3 (more than 20MB) and for slow connections that can become a really huge problem since the user could decide to load another model while the loading still was in progress, I currently have no way to stop the download, so new loadings will get even slower and the only way to stop that is to reload the page.

So the bigger problem to solve imho is to allow aborting the network request, the model loading, which seems the most discussed part here, isn't really important.

Hope this could get some attention from maintainers (@mrdoob @Mugen87 @gkjohnson) to settle an API so that @mistic100 could prepare this to get merged.