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

originalRequest and originalResponse in DetailesError are stored as reference #448

Open
AlexAndBear opened this issue Aug 25, 2022 · 3 comments
Labels

Comments

@AlexAndBear
Copy link

AlexAndBear commented Aug 25, 2022

Describe the bug

this.originalRequest = req
reqand res will be stored as a reference in originalRequest and originalResponse.

This is quite cool, as we can access the response, get the status and do proper error handling in the client to show a human-readable error message.

However, using uppy with tus-js-client as plugin will call abort on the request in the error case, which will set the readyState but also the status to 0
(see: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/abort)

So I won't be able to read the status code in my underlying code.

Is it possible to store req and res as a deep copy, for proper error handling?

@Acconut
Copy link
Member

Acconut commented Sep 9, 2022

Thank you for this report. I am a bit split on this topic. On the one hand, I see the reason for why you would not want to modify the error after it has been raised. On the other, I can imagine there are situations where you want to have a reference and use it for further diagnostic measures. In addition, making a deep copy might cause more confusion for users because that is not what they would expect.

However, using uppy with tus-js-client as plugin will call abort on the request in the error case, which will set the readyState but also the status to 0

Maybe it is possible to fix this inside Uppy, so that abort does not get called? I could not think of a specific need for this call after an error has been raised. Could you bring this issue to the Uppy repository?

@mifi
Copy link
Collaborator

mifi commented Sep 12, 2022

It looks like Uppy aborts tus when it encounters any error (could also be an error unrelated to Tus I think?) e.g. the onError option:
https://github.com/transloadit/uppy/blob/952fb8dab3530bc04d4dc7694bd7d44f4aae8874/packages/%40uppy/tus/src/index.js#L262

Then it calls the cleanup function resetUploaderReferences which aborts the tus upload. If it were not to call this function, I think it could cause memory leaks and upload continuing in the background even after error.

Maybe if Tus had a boolean property like isRunning or hasFailed (I couldn't find any such property), then we could check that property first and only abort if tus has not failed, e.g. in resetUploaderReferences:

if (!uploader.hasFailed) uploader.abort();

What do you think?

@Acconut
Copy link
Member

Acconut commented Sep 14, 2022

Maybe if Tus had a boolean property like isRunning or hasFailed (I couldn't find any such property), then we could check that property first and only abort if tus has not failed, e.g. in resetUploaderReferences:

In #453 we talked about exposing such properties after an internal modification of tus-js-client to use a state machine. So I hope to add such properties soon, but there is no ETA available right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants