Open JohnDennehy101 opened 6 months ago
Hi @Murderlon @aduh95, thanks for assigning for further investigation. Just wondering if ye have had a chance to take a look in more detail / will this issue be resolved in a future release?
This is also happening to me. I blocked the outgoing aws connection to get a timeout error to test the upload-error
hook but it never triggers the hook.
Hello, I'm having trouble reproducing, are you able to reproduce consistently the issue or is there some flakiness involved? Are you trying to upload local files, or files from a remote provider (e.g. Google Drive)? What is the behavior on the DevTools? Do you see an infinite amount of requests being sent (and at what interval are they sent), or is it just one request pending indefinitely?
For the record, I tried to reproduce using yarn dev:with-companion
:
uppyDashboard.on('upload-error', function(...args) {debugger})
in private/dev/Dashboard.js
to detect if the event is emitted.https://s3.us-east-1.amazonaws.com/*
requests.This is also happening to me. I blocked the outgoing aws connection to get a timeout error to test the
upload-error
hook but it never triggers the hook.
Can you clarify if you block the outgoing connections from the client or from Companion side?
Hi @aduh95,
I have resolved this issue by hardcoding a custom plugin using the code before the change I highlighted was implemented.
I was able to consistently reproduce the issue at the time. Our use case is for uploading large local files to s3 using pre-signed urls returned from companion.
On loss of network connection, the pending requests just remain in that state and the browser just hangs (with the upload progress spinner still displaying that the upload is in progress).
We do not block the outgoing connections from either side.
I guess emulating loss of connexion is simply hard, I don't know how to do that but by blocking outgoing requests. Without a repro, I hope you understand it's quite hard to work on fixing it.
Initial checklist
Link to runnable example
No response
Steps to reproduce
The overview of our current configuration can be seen below.
The front-end React application used by users to provide files for upload
"@uppy/aws-s3-multipart": "^3.7.0", "@uppy/core": "^3.6.1", "@uppy/dashboard": "^3.6.0", "@uppy/drag-drop": "^3.0.3", "@uppy/file-input": "^3.0.4", "@uppy/progress-bar": "^3.0.4", "@uppy/react": "^3.1.4",`
This is used in conjunction with Uppy Companion on the back-end. The package from Uppy used in this service is:
"@uppy/companion": "^4.9.0",
The basic configuration there is
I have never had an issue with Uppy before and find it a great service that is provided via open source. Just spotted this as one of our clients mentioned an upload was hanging indefinitely in the browser and after a lot of debugging I was able to narrow it down to a change in the code in the aws-s3-multipart package.
Expected behavior
The upload-error event should be triggered if the upload fails beyond allotted retryDelays provided to AwsS3Multipart.
Actual behavior
The browser hangs indefinitely and the upload-error event is not triggered on loss of network connection. It looks to me like the requests are not being retried according to the provided retryInterval configuration.
I have traced this issue to this change - https://github.com/transloadit/uppy/pull/4691
I have been able to resolve this by reverting the code changes in https://github.com/transloadit/uppy/pull/4691 in the aws-s3-multipart package.
When I use the below code (code with the changes in https://github.com/transloadit/uppy/pull/4691 reverted), the upload-error event is triggered after all of the retryIntervals have been exceeded.
all plugin code
```js import BasePlugin from "@uppy/core/lib/BasePlugin.js"; import { RequestClient } from "@uppy/companion-client"; import EventManager from "@uppy/utils/lib/EventManager"; import { RateLimitedQueue } from "@uppy/utils/lib/RateLimitedQueue"; import { filterNonFailedFiles, filterFilesToEmitUploadStarted, } from "@uppy/utils/lib/fileFilters"; import { createAbortError } from "@uppy/utils/lib/AbortController"; import MultipartUploader, { pausingUploadReason } from "./MultipartUploader.js"; import createSignedURL from "./createSignedURL.js"; function assertServerError(res) { if (res && res.error) { const error = new Error(res.message); Object.assign(error, res.error); throw error; } return res; } function removeMetadataFromURL(urlString) { const urlObject = new URL(urlString); urlObject.search = ""; urlObject.hash = ""; return urlObject.href; } /** * Computes the expiry time for a request signed with temporary credentials. If * no expiration was provided, or an invalid value (e.g. in the past) is * provided, undefined is returned. This function assumes the client clock is in * sync with the remote server, which is a requirement for the signature to be * validated for AWS anyway. * * @param {import('../types/index.js').AwsS3STSResponse['credentials']} credentials * @returns {number | undefined} */ function getExpiry(credentials) { const expirationDate = credentials.Expiration; if (expirationDate) { const timeUntilExpiry = Math.floor( (new Date(expirationDate) - Date.now()) / 1000 ); if (timeUntilExpiry > 9) { return timeUntilExpiry; } } return undefined; } function getAllowedMetadata({ meta, allowedMetaFields, querify = false }) { const metaFields = allowedMetaFields ?? Object.keys(meta); if (!meta) return {}; return Object.fromEntries( metaFields .filter((key) => meta[key] != null) .map((key) => { const realKey = querify ? `metadata[${key}]` : key; const value = String(meta[key]); return [realKey, value]; }) ); } function throwIfAborted(signal) { if (signal?.aborted) { throw createAbortError("The operation was aborted", { cause: signal.reason, }); } } class HTTPCommunicationQueue { #abortMultipartUpload; #cache = new WeakMap(); #createMultipartUpload; #fetchSignature; #getUploadParameters; #listParts; #previousRetryDelay; #requests; #retryDelayIterator; #sendCompletionRequest; #setS3MultipartState; #uploadPartBytes; #getFile; constructor(requests, options, setS3MultipartState, getFile) { this.#requests = requests; this.#setS3MultipartState = setS3MultipartState; this.#getFile = getFile; this.setOptions(options); } setOptions(options) { const requests = this.#requests; if ("abortMultipartUpload" in options) { this.#abortMultipartUpload = requests.wrapPromiseFunction( options.abortMultipartUpload, { priority: 1 } ); } if ("createMultipartUpload" in options) { this.#createMultipartUpload = requests.wrapPromiseFunction( options.createMultipartUpload, { priority: -1 } ); } if ("signPart" in options) { this.#fetchSignature = requests.wrapPromiseFunction(options.signPart); } if ("listParts" in options) { this.#listParts = requests.wrapPromiseFunction(options.listParts); } if ("completeMultipartUpload" in options) { this.#sendCompletionRequest = requests.wrapPromiseFunction( options.completeMultipartUpload, { priority: 1 } ); } if ("retryDelays" in options) { this.#retryDelayIterator = options.retryDelays?.values(); } if ("uploadPartBytes" in options) { this.#uploadPartBytes = requests.wrapPromiseFunction( options.uploadPartBytes, { priority: Infinity } ); } if ("getUploadParameters" in options) { this.#getUploadParameters = requests.wrapPromiseFunction( options.getUploadParameters ); } } async #shouldRetry(err) { const requests = this.#requests; const status = err?.source?.status; // TODO: this retry logic is taken out of Tus. We should have a centralized place for retrying, // perhaps the rate limited queue, and dedupe all plugins with that. if (status == null) { return false; } if (status === 403 && err.message === "Request has expired") { if (!requests.isPaused) { // We don't want to exhaust the retryDelayIterator as long as there are // more than one request in parallel, to give slower connection a chance // to catch up with the expiry set in Companion. if (requests.limit === 1 || this.#previousRetryDelay == null) { const next = this.#retryDelayIterator?.next(); if (next == null || next.done) { return false; } // If there are more than 1 request done in parallel, the RLQ limit is // decreased and the failed request is requeued after waiting for a bit. // If there is only one request in parallel, the limit can't be // decreased, so we iterate over `retryDelayIterator` as we do for // other failures. // `#previousRetryDelay` caches the value so we can re-use it next time. this.#previousRetryDelay = next.value; } // No need to stop the other requests, we just want to lower the limit. requests.rateLimit(0); await new Promise((resolve) => setTimeout(resolve, this.#previousRetryDelay) ); } } else if (status === 429) { // HTTP 429 Too Many Requests => to avoid the whole download to fail, pause all requests. if (!requests.isPaused) { const next = this.#retryDelayIterator?.next(); if (next == null || next.done) { return false; } requests.rateLimit(next.value); } } else if (status > 400 && status < 500 && status !== 409) { // HTTP 4xx, the server won't send anything, it's doesn't make sense to retry return false; } else if (typeof navigator !== "undefined" && navigator.onLine === false) { // The navigator is offline, let's wait for it to come back online. if (!requests.isPaused) { requests.pause(); window.addEventListener( "online", () => { requests.resume(); }, { once: true } ); } } else { // Other error code means the request can be retried later. const next = this.#retryDelayIterator?.next(); if (next == null || next.done) { return false; } await new Promise((resolve) => setTimeout(resolve, next.value)); } return true; } async getUploadId(file, signal) { let cachedResult; // As the cache is updated asynchronously, there could be a race condition // where we just miss a new result so we loop here until we get nothing back, // at which point it's out turn to create a new cache entry. while ((cachedResult = this.#cache.get(file.data)) != null) { try { return await cachedResult; } catch { // In case of failure, we want to ignore the cached error. // At this point, either there's a new cached value, or we'll exit the loop a create a new one. } } const promise = this.#createMultipartUpload(this.#getFile(file), signal); const abortPromise = () => { promise.abort(signal.reason); this.#cache.delete(file.data); }; signal.addEventListener("abort", abortPromise, { once: true }); this.#cache.set(file.data, promise); promise.then( async (result) => { signal.removeEventListener("abort", abortPromise); this.#setS3MultipartState(file, result); this.#cache.set(file.data, result); }, () => { signal.removeEventListener("abort", abortPromise); this.#cache.delete(file.data); } ); return promise; } async abortFileUpload(file) { const result = this.#cache.get(file.data); if (result == null) { // If the createMultipartUpload request never was made, we don't // need to send the abortMultipartUpload request. return; } // Remove the cache entry right away for follow-up requests do not try to // use the soon-to-be aborted chached values. this.#cache.delete(file.data); this.#setS3MultipartState(file, Object.create(null)); let awaitedResult; try { awaitedResult = await result; } catch { // If the cached result rejects, there's nothing to abort. return; } await this.#abortMultipartUpload(this.#getFile(file), awaitedResult); } async #nonMultipartUpload(file, chunk, signal) { const { method = "POST", url, fields, headers, } = await this.#getUploadParameters(this.#getFile(file), { signal, }).abortOn(signal); let body; const data = chunk.getData(); if (method.toUpperCase() === "POST") { const formData = new FormData(); Object.entries(fields).forEach(([key, value]) => formData.set(key, value) ); formData.set("file", data); body = formData; } else { body = data; } const { onProgress, onComplete } = chunk; const result = await this.#uploadPartBytes({ signature: { url, headers, method }, body, size: data.size, onProgress, onComplete, signal, }).abortOn(signal); return "location" in result ? result : { location: removeMetadataFromURL(url), ...result, }; } /** * @param {import("@uppy/core").UppyFile} file * @param {import("../types/chunk").Chunk[]} chunks * @param {AbortSignal} signal * @returns {Promise