From 6d086076c092c6fb433e7cf3abcb22b85accdc5f Mon Sep 17 00:00:00 2001 From: cosa65 Date: Fri, 17 Nov 2023 12:42:03 -0300 Subject: [PATCH 1/8] Update axios to 1.6.0 so we can use the abortController Signed-off-by: cosa65 --- package-lock.json | 22 ++++++++++++++++++---- package.json | 2 +- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 0745c4a56b..f9660c9faf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13123,11 +13123,25 @@ "dev": true }, "axios": { - "version": "0.21.4", - "resolved": "https://registry.npmjs.org/axios/-/axios-0.21.4.tgz", - "integrity": "sha512-ut5vewkiu8jjGBdqpM44XxjuCjq9LAKeHVmoVfHVzy8eHgxxq8SbAVQNovDA8mVi05kP0Ea/n/UzcSHcTJQfNg==", + "version": "1.6.0", + "resolved": "https://registry.npmjs.org/axios/-/axios-1.6.0.tgz", + "integrity": "sha512-EZ1DYihju9pwVB+jg67ogm+Tmqc6JmhamRN6I4Zt8DfZu5lbcQGw3ozH9lFejSJgs/ibaef3A9PMXPLeefFGJg==", "requires": { - "follow-redirects": "^1.14.0" + "follow-redirects": "^1.15.0", + "form-data": "^4.0.0", + "proxy-from-env": "^1.1.0" + }, + "dependencies": { + "form-data": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.0.tgz", + "integrity": "sha512-ETEklSGi5t0QMZuiXoA/Q6vcnxcLQP5vdugSpuAyi6SVGi2clPPp+xgEhuMaHC+zGgn31Kd235W35f7Hykkaww==", + "requires": { + "asynckit": "^0.4.0", + "combined-stream": "^1.0.8", + "mime-types": "^2.1.12" + } + } } }, "axobject-query": { diff --git a/package.json b/package.json index 35d1322a8d..3db8df2cd0 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "@vitessce/scatterplot": "^3.2.2", "antd": "^4.24.8", "aws-amplify": "^5.3.11", - "axios": "^0.21.2", + "axios": "1.6.0", "babel-plugin-add-module-exports": "^1.0.4", "babel-plugin-dynamic-import-node": "^2.3.3", "babel-plugin-import": "^1.13.6", From ec63ef23ccacc0ef57f45f1f2d62d70d31fca4e3 Mon Sep 17 00:00:00 2001 From: cosa65 Date: Fri, 17 Nov 2023 14:14:36 -0300 Subject: [PATCH 2/8] Fix wrong names being used whencreating sample files, they are not the standardized ones so we can end up with duplicated pieces of data for the same files Signed-off-by: cosa65 --- src/redux/actions/samples/createSamples.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/redux/actions/samples/createSamples.js b/src/redux/actions/samples/createSamples.js index 18c51da74b..ccbad5ab71 100644 --- a/src/redux/actions/samples/createSamples.js +++ b/src/redux/actions/samples/createSamples.js @@ -14,6 +14,9 @@ import { defaultSampleOptions, sampleTemplate } from 'redux/reducers/samples/ini import { sampleTech } from 'utils/constants'; import UploadStatus from 'utils/upload/UploadStatus'; +import fileNameForApiV1 from 'utils/upload/fileNameForApiV1'; +import getFileTypeV2 from 'utils/getFileTypeV2'; + // If the sample name of new samples coincides with already existing // ones we should not create new samples, // just reuse their sampleIds and upload the new files @@ -115,9 +118,13 @@ const createSamples = ( options, metadata: experiment?.metadataKeys .reduce((acc, curr) => ({ ...acc, [curr]: METADATA_DEFAULT_VALUE }), {}) || {}, - files: Object.values(files).reduce(((acc, curr) => ( - { ...acc, [curr.name]: { upload: { status: UploadStatus.UPLOADING } } } - )), {}), + files: Object.values(files).reduce(((acc, curr) => { + const fileType = fileNameForApiV1[getFileTypeV2(curr.name, sampleTechnology)]; + + return ( + { ...acc, [fileType]: { upload: { status: UploadStatus.UPLOADING } } } + ); + }), {}), })); dispatch({ From 625fc6baf727364810d0a3210d12aa65016cc3a3 Mon Sep 17 00:00:00 2001 From: cosa65 Date: Fri, 17 Nov 2023 14:19:17 -0300 Subject: [PATCH 3/8] Add abortController, pass it to both the upload process and redux Signed-off-by: cosa65 --- src/redux/actions/samples/createSampleFile.js | 5 ++++- src/utils/upload/processMultipartUpload.js | 16 +++++++++++++--- src/utils/upload/processUpload.js | 12 ++++++++---- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/redux/actions/samples/createSampleFile.js b/src/redux/actions/samples/createSampleFile.js index ec91f235a0..41de796303 100644 --- a/src/redux/actions/samples/createSampleFile.js +++ b/src/redux/actions/samples/createSampleFile.js @@ -13,6 +13,7 @@ const createSampleFile = ( sampleId, type, fileForApiV1, + abortController, ) => async (dispatch) => { const updatedAt = dayjs().toISOString(); @@ -30,8 +31,10 @@ const createSampleFile = ( lastModified: updatedAt, fileName: fileNameForApiV1[type], fileDiff: { - upload: { status: UploadStatus.UPLOADING }, ...fileForApiV1, + upload: { + status: UploadStatus.UPLOADING, progress: 0, abortController, + }, }, }, }); diff --git a/src/utils/upload/processMultipartUpload.js b/src/utils/upload/processMultipartUpload.js index 5e1bae0c03..e3c34fc480 100644 --- a/src/utils/upload/processMultipartUpload.js +++ b/src/utils/upload/processMultipartUpload.js @@ -3,12 +3,13 @@ import axios from 'axios'; const FILE_CHUNK_SIZE = 10000000; const MAX_RETRIES = 2; -const putPartInS3 = async (blob, signedUrl, onUploadProgress, currentRetry = 0) => { +const putPartInS3 = async (blob, signedUrl, onUploadProgress, currentRetry = 0, abortController = null) => { try { return await axios.request({ method: 'put', data: blob, url: signedUrl, + signal: abortController, headers: { 'Content-Type': 'application/octet-stream', }, @@ -23,7 +24,9 @@ const putPartInS3 = async (blob, signedUrl, onUploadProgress, currentRetry = 0) } }; -const processMultipartUpload = async (file, signedUrls, createOnUploadProgressForPart) => { +const processMultipartUpload = async ( + file, signedUrls, createOnUploadProgressForPart, abortController = null, +) => { const promises = []; signedUrls.forEach((signedUrl, index) => { @@ -33,7 +36,14 @@ const processMultipartUpload = async (file, signedUrls, createOnUploadProgressFo ? file.fileObject.slice(start, end) : file.fileObject.slice(start); - const req = putPartInS3(blob, signedUrl, createOnUploadProgressForPart(index)); + const req = putPartInS3( + blob, + signedUrl, + createOnUploadProgressForPart(index), + 0, + abortController, + ); + promises.push(req); }); diff --git a/src/utils/upload/processUpload.js b/src/utils/upload/processUpload.js index 607be41d30..12a850f13e 100644 --- a/src/utils/upload/processUpload.js +++ b/src/utils/upload/processUpload.js @@ -18,7 +18,7 @@ import endUserMessages from 'utils/endUserMessages'; import pushNotificationMessage from 'utils/pushNotificationMessage'; const prepareAndUploadFileToS3 = async ( - file, uploadUrlParams, type, onStatusUpdate = () => { }, + file, uploadUrlParams, type, onStatusUpdate = () => { }, abortController, ) => { let parts = null; const { signedUrls, uploadId, fileId } = uploadUrlParams; @@ -34,7 +34,9 @@ const prepareAndUploadFileToS3 = async ( onStatusUpdate(UploadStatus.UPLOADING, percentProgress); }; try { - parts = await processMultipartUpload(file, signedUrls, createOnUploadProgressForPart); + parts = await processMultipartUpload( + file, signedUrls, createOnUploadProgressForPart, abortController, + ); } catch (e) { onStatusUpdate(UploadStatus.UPLOAD_ERROR); return; @@ -64,6 +66,8 @@ const prepareAndUploadFileToS3 = async ( const createAndUploadSampleFile = async (file, experimentId, sampleId, dispatch, selectedTech) => { const fileType = getFileTypeV2(file.name, selectedTech); + const abortController = new AbortController(); + let sampleFileId; try { @@ -73,6 +77,7 @@ const createAndUploadSampleFile = async (file, experimentId, sampleId, dispatch, sampleId, fileType, file, + abortController, ), ); } catch (e) { @@ -114,8 +119,7 @@ const createAndUploadSampleFile = async (file, experimentId, sampleId, dispatch, ); const uploadUrlParams = { signedUrls, uploadId, fileId: sampleFileId }; - - await prepareAndUploadFileToS3(file, uploadUrlParams, 'sample', updateSampleFileUploadProgress); + await prepareAndUploadFileToS3(file, uploadUrlParams, 'sample', updateSampleFileUploadProgress, abortController); } catch (e) { dispatch(updateSampleFileUpload( experimentId, sampleId, fileType, UploadStatus.UPLOAD_ERROR, From 339b476715ce1efed66d84351ea2ba728da2ea11 Mon Sep 17 00:00:00 2001 From: cosa65 Date: Wed, 22 Nov 2023 10:00:41 -0300 Subject: [PATCH 4/8] Fix, pass abort signal instead of controller to axios and also address occasional racing condition between sample file updates and sample delete Signed-off-by: cosa65 --- src/redux/actions/samples/deleteSamples.js | 14 +++++--------- src/redux/reducers/samples/samplesFileUpdate.js | 6 ++++++ src/utils/upload/processMultipartUpload.js | 6 ++++-- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/redux/actions/samples/deleteSamples.js b/src/redux/actions/samples/deleteSamples.js index 29c2dbce6a..7e9702bffb 100644 --- a/src/redux/actions/samples/deleteSamples.js +++ b/src/redux/actions/samples/deleteSamples.js @@ -11,15 +11,11 @@ import endUserMessages from 'utils/endUserMessages'; import fetchAPI from 'utils/http/fetchAPI'; import handleError from 'utils/http/handleError'; -const cancelUploads = async (files) => { - const promises = Object.values(files).map(({ upload }) => { - if (upload?.amplifyPromise) { - // return Storage.cancel(upload.amplifyPromise); - } - return Promise.resolve(); +const cancelUploads = (files) => { + Object.values(files).forEach((file) => { + // eslint-disable-next-line no-unused-expressions + file.upload?.abortController?.abort(); }); - - return Promise.all(promises); }; const deleteSamples = ( @@ -34,7 +30,7 @@ const deleteSamples = ( acc[samples[sampleUuid].experimentId] = []; } - await cancelUploads(files); + cancelUploads(files); return { ...acc, diff --git a/src/redux/reducers/samples/samplesFileUpdate.js b/src/redux/reducers/samples/samplesFileUpdate.js index a88dac578a..c9c1176672 100644 --- a/src/redux/reducers/samples/samplesFileUpdate.js +++ b/src/redux/reducers/samples/samplesFileUpdate.js @@ -5,6 +5,12 @@ const samplesFileUpdate = (state, action) => { sampleUuid, fileName, fileDiff, lastModified, } = action.payload; + // There's a possible racing condition where a file update can reach this place + // after a sample is deleted and there's a crash. This check is in place to avoid that error. + if (_.isNil(state[sampleUuid])) { + return state; + } + const oldFile = state[sampleUuid].files?.[fileName]; let newFile = fileDiff; diff --git a/src/utils/upload/processMultipartUpload.js b/src/utils/upload/processMultipartUpload.js index e3c34fc480..1e1a10e8b8 100644 --- a/src/utils/upload/processMultipartUpload.js +++ b/src/utils/upload/processMultipartUpload.js @@ -3,13 +3,15 @@ import axios from 'axios'; const FILE_CHUNK_SIZE = 10000000; const MAX_RETRIES = 2; -const putPartInS3 = async (blob, signedUrl, onUploadProgress, currentRetry = 0, abortController = null) => { +const putPartInS3 = async ( + blob, signedUrl, onUploadProgress, currentRetry = 0, abortController = null, +) => { try { return await axios.request({ method: 'put', data: blob, url: signedUrl, - signal: abortController, + signal: abortController.signal, headers: { 'Content-Type': 'application/octet-stream', }, From 2e110861d229cdf4e44120657b18df433417eba4 Mon Sep 17 00:00:00 2001 From: cosa65 Date: Wed, 22 Nov 2023 10:53:23 -0300 Subject: [PATCH 5/8] Update snapshots Signed-off-by: cosa65 --- .../upload/__snapshots__/process10XUpload.test.js.snap | 6 ++++++ .../upload/__snapshots__/processSeuratUpload.test.js.snap | 2 ++ 2 files changed, 8 insertions(+) diff --git a/src/__test__/utils/upload/__snapshots__/process10XUpload.test.js.snap b/src/__test__/utils/upload/__snapshots__/process10XUpload.test.js.snap index 8d69fa1852..05b70f64d2 100644 --- a/src/__test__/utils/upload/__snapshots__/process10XUpload.test.js.snap +++ b/src/__test__/utils/upload/__snapshots__/process10XUpload.test.js.snap @@ -234,6 +234,7 @@ exports[`processUpload Uploads and updates redux correctly when there are no err }, "method": "put", "onUploadProgress": [Function], + "signal": AbortSignal {}, "url": "theSignedUrl", }, { @@ -243,6 +244,7 @@ exports[`processUpload Uploads and updates redux correctly when there are no err }, "method": "put", "onUploadProgress": [Function], + "signal": AbortSignal {}, "url": "theSignedUrl", }, { @@ -252,6 +254,7 @@ exports[`processUpload Uploads and updates redux correctly when there are no err }, "method": "put", "onUploadProgress": [Function], + "signal": AbortSignal {}, "url": "theSignedUrl", }, ] @@ -401,6 +404,7 @@ exports[`processUpload Uploads and updates redux correctly when there are no err }, "method": "put", "onUploadProgress": [Function], + "signal": AbortSignal {}, "url": "theSignedUrl", }, { @@ -410,6 +414,7 @@ exports[`processUpload Uploads and updates redux correctly when there are no err }, "method": "put", "onUploadProgress": [Function], + "signal": AbortSignal {}, "url": "theSignedUrl", }, { @@ -419,6 +424,7 @@ exports[`processUpload Uploads and updates redux correctly when there are no err }, "method": "put", "onUploadProgress": [Function], + "signal": AbortSignal {}, "url": "theSignedUrl", }, ] diff --git a/src/__test__/utils/upload/__snapshots__/processSeuratUpload.test.js.snap b/src/__test__/utils/upload/__snapshots__/processSeuratUpload.test.js.snap index c76a9dce9f..42209cc94e 100644 --- a/src/__test__/utils/upload/__snapshots__/processSeuratUpload.test.js.snap +++ b/src/__test__/utils/upload/__snapshots__/processSeuratUpload.test.js.snap @@ -9,6 +9,7 @@ exports[`processUpload Uploads and updates redux correctly when there are no err }, "method": "put", "onUploadProgress": [Function], + "signal": AbortSignal {}, "url": "theSignedUrl1", }, { @@ -18,6 +19,7 @@ exports[`processUpload Uploads and updates redux correctly when there are no err }, "method": "put", "onUploadProgress": [Function], + "signal": AbortSignal {}, "url": "theSignedUrl2", }, ] From 5c57f16eebc918530ca3dc8c1d483250837f2247 Mon Sep 17 00:00:00 2001 From: cosa65 Date: Thu, 23 Nov 2023 09:31:29 -0300 Subject: [PATCH 6/8] Add test for sample file update on sample not existing anymore Signed-off-by: cosa65 --- src/__test__/redux/reducers/samplesReducer.test.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/__test__/redux/reducers/samplesReducer.test.js b/src/__test__/redux/reducers/samplesReducer.test.js index 8b82193195..780b919e6f 100644 --- a/src/__test__/redux/reducers/samplesReducer.test.js +++ b/src/__test__/redux/reducers/samplesReducer.test.js @@ -398,4 +398,18 @@ describe('samplesReducer', () => { expect(newState).toMatchSnapshot(); }); + + it('Sample file update doesnt change anything if the sample no longer exists', () => { + const newState = samplesReducer(initialState, { + type: SAMPLES_FILE_UPDATE, + payload: { + sampleUuid: mockUuid1, + fileName, + fileDiff: mockFile, + lastModified: 'newLastModified', + }, + }); + + expect(newState).toEqual(initialState); + }); }); From 950966c17127b6cd84d66ef0db460c4a734624ef Mon Sep 17 00:00:00 2001 From: cosa65 Date: Tue, 28 Nov 2023 08:39:46 -0300 Subject: [PATCH 7/8] Fix word Signed-off-by: cosa65 --- src/redux/reducers/samples/samplesFileUpdate.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/redux/reducers/samples/samplesFileUpdate.js b/src/redux/reducers/samples/samplesFileUpdate.js index c9c1176672..ee1561f15f 100644 --- a/src/redux/reducers/samples/samplesFileUpdate.js +++ b/src/redux/reducers/samples/samplesFileUpdate.js @@ -5,7 +5,7 @@ const samplesFileUpdate = (state, action) => { sampleUuid, fileName, fileDiff, lastModified, } = action.payload; - // There's a possible racing condition where a file update can reach this place + // There's a possible race condition where a file update can reach this place // after a sample is deleted and there's a crash. This check is in place to avoid that error. if (_.isNil(state[sampleUuid])) { return state; From 496e0294e5ac91eee88833e738235e37b8adf684 Mon Sep 17 00:00:00 2001 From: cosa65 Date: Tue, 28 Nov 2023 08:41:01 -0300 Subject: [PATCH 8/8] Move abortController default value definition Signed-off-by: cosa65 --- src/utils/upload/processMultipartUpload.js | 2 +- src/utils/upload/processUpload.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/upload/processMultipartUpload.js b/src/utils/upload/processMultipartUpload.js index 1e1a10e8b8..c1c0e3d97e 100644 --- a/src/utils/upload/processMultipartUpload.js +++ b/src/utils/upload/processMultipartUpload.js @@ -27,7 +27,7 @@ const putPartInS3 = async ( }; const processMultipartUpload = async ( - file, signedUrls, createOnUploadProgressForPart, abortController = null, + file, signedUrls, createOnUploadProgressForPart, abortController, ) => { const promises = []; diff --git a/src/utils/upload/processUpload.js b/src/utils/upload/processUpload.js index 12a850f13e..8c3e4d70b2 100644 --- a/src/utils/upload/processUpload.js +++ b/src/utils/upload/processUpload.js @@ -18,7 +18,7 @@ import endUserMessages from 'utils/endUserMessages'; import pushNotificationMessage from 'utils/pushNotificationMessage'; const prepareAndUploadFileToS3 = async ( - file, uploadUrlParams, type, onStatusUpdate = () => { }, abortController, + file, uploadUrlParams, type, onStatusUpdate = () => { }, abortController = null, ) => { let parts = null; const { signedUrls, uploadId, fileId } = uploadUrlParams;