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

Ability to cancel ongoing HTTP requests in loaders #23070

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

mistic100
Copy link
Contributor

@mistic100 mistic100 commented Dec 22, 2021

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 mrdoob changed the title Fix #20705 Ability to cancel ongoing HTTP requests in loaders Ability to cancel ongoing HTTP requests in loaders Dec 22, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 22, 2021

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

@gkjohnson
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Contributor Author

mistic100 commented Jan 8, 2022

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

mistic100 commented Jan 8, 2022

@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"

Copy link
Collaborator

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other maintainers will have to weigh in on whether the Loader.setAbortLoader API is acceptable but in my opinion this fits with the existing APIs on the Loaders.

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.

src/loaders/FileLoader.js Show resolved Hide resolved
src/loaders/ImageBitmapLoader.js Show resolved Hide resolved
@mistic100
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@@ -124,6 +125,7 @@ class Rhino3dmLoader extends Loader {

worker._callbacks[ taskID ] = { resolve, reject };

// TODO if abortSignal is defined, listen to it to cancel the worker
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally the abort signal would be handled to "kill" the web worker. having never used web worker I prefer not to try it myself.

@@ -1348,6 +1380,9 @@ class MaterialBuilder {

}

this.tgaLoader.setWithCredentials( this.crossOrigin === 'use-credentials' );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same logic is already used in GLTFLoader

@mistic100
Copy link
Contributor Author

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

@rhuitl
Copy link
Contributor

rhuitl commented Jan 27, 2022

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
Copy link
Contributor

Any updates on the review @gkjohnson ?

@mrdoob mrdoob modified the milestones: r157, r158 Sep 28, 2023
@alessiocancian
Copy link

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

@mrdoob mrdoob modified the milestones: r158, r159 Oct 27, 2023
@alessiocancian
Copy link

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.

@alanb-sony
Copy link

@alessiocancian I'm in a similar situation, the temporary bodge I've used is to do the fetching myself into a Blob then use createObjectURL to create a URL to pass to the three loader. Obviously this isn't ideal and only works for single file models.

@rhuitl
Copy link
Contributor

rhuitl commented Nov 6, 2023

For single file loads (e.g. textures), my workaround is to monkey-patch the fetch() method to include the abort controller. But that only works if the load() method calls fetch() synchronously.

const originalFetch = window.fetch;
window.fetch = (input: RequestInfo | URL, init ? : RequestInit): Promise < Response > => {
    init.signal = abortController?.signal;
    return originalFetch(input, init);
};

try {
    return textureLoader.load(/*...*/);
} finally {
    window.fetch = originalFetch;
}

@mrdoob mrdoob modified the milestones: r159, r160 Nov 30, 2023
@mrdoob mrdoob modified the milestones: r160, r161 Dec 22, 2023
@mrdoob mrdoob modified the milestones: r161, r162 Jan 31, 2024
Co-authored-by: Levi Pesin <35454228+LeviPesin@users.noreply.github.com>
@LeviPesin
Copy link
Contributor

@mrdoob @Mugen87 Is there anything preventing merging this PR?

@mrdoob mrdoob modified the milestones: r162, r163 Feb 29, 2024
@mrdoob mrdoob modified the milestones: r163, r164 Mar 29, 2024
@mrdoob mrdoob modified the milestones: r164, r165 Apr 25, 2024
@mrdoob mrdoob modified the milestones: r165, r166 May 31, 2024
@mrdoob mrdoob modified the milestones: r166, r167 Jun 28, 2024
@mrdoob mrdoob modified the milestones: r167, r168 Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet