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

@uppy/provider-views: fix race condition when adding folders #4384

Merged
merged 26 commits into from
Apr 4, 2023

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Mar 28, 2023

don't call addFolder from listAllFiles
because that would call addFile before all folders were loaded

also remove unused selectedFolders state

update: this also fixes #4381

update 2:

The problem still exists even with this rewrite, if you add separate 2 folders (e.g. 2 folder with a checkbox on each folder)

Screenshot 2023-03-28 at 23 30 22

update 3: have pushed a commit to fix selecting multiple folders, but problem still persists if you select files along with folders (using checkboxes), because setFile gets called instantly for files first, then folders will load after that

update 4: turns out this PR also seems to fix #4373

update 5: ok now I have also fixed the problem from update 3, and removed loaderWrapper, improve loading message, fixed broken duplicate files check

don't call addFolder from listAllFiles
because that would call addFile before all folders were loaded

also remove unused selectedFolders state
@socket-security
Copy link

socket-security bot commented Mar 28, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No dependency changes detected in pull request

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround! 👏

@Murderlon Murderlon changed the title fix race condtion when adding files @uppy/provider-views: fix race condition when adding folders Mar 28, 2023
@arturi arturi self-requested a review March 28, 2023 21:54
@mifi
Copy link
Contributor Author

mifi commented Mar 29, 2023

I have no clue how that commit lead to that build failure

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Very nice that you were able to get rid of the SharedHandler 👏

@@ -164,6 +164,7 @@ en_US.strings = {
takePicture: 'Take a picture',
takePictureBtn: 'Take Picture',
timedOut: 'Upload stalled for %{seconds} seconds, aborting.',
traversingSubfolders: 'Traversing %{numSubFolders} subfolder(s) inside "%{folder}"',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to inform the user abut this? I think adding a certain folder is a conscious decision that it comes with all files, including the ones in a subfolder. The term "traversing" is also tech jargon and not as user friendly. I would propose the remove it.

Copy link
Contributor Author

@mifi mifi Mar 29, 2023

Choose a reason for hiding this comment

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

if a user selects a folder structure that has a lot of files and folders inside of it, then it will take a long time, so i thought it makes sense to show the user that something is happening, or else they might think that it has crashed if it says just "loading" for many minutes. kind of like how all file copy progress dialogs in windows, macos etc show that they are copying something. but maybe we can show it some other way? we cannot show a percentage because we don't know how ahead of time how deep the folder structure might be

Copy link
Member

Choose a reason for hiding this comment

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

The difference in those mentioned dialogs is that they show progress, while we show a static message on top of a static "loading" message. I'm not sure if it adds much value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not, but I think at least we shoulf show something. Maybe we can show the number of files loaded instead?

Copy link
Member

Choose a reason for hiding this comment

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

I would remove this from now and in a separate PR we can explore the improvement? Perhaps a message indeed like Loaded {amount} files which updates.

packages/@uppy/core/src/Uppy.js Outdated Show resolved Hide resolved
mifi added a commit that referenced this pull request Mar 29, 2023
while loading
this lets the user know that we are doing something, and that the process has not crashed or gotten stuck
see #4384
@mifi
Copy link
Contributor Author

mifi commented Mar 29, 2023

hmm, i reverted the commits but the build is still failing. so definitely something wrong with https://github.com/transloadit/uppy/blob/main/.github/workflows/bundlers.yml

@arturi
Copy link
Contributor

arturi commented Mar 29, 2023

This will conflict now with https://github.com/transloadit/uppy/pull/4283/files somewhat, but I'll see how bad it is. Would be nice to release both as a Provider refactor.

@transloadit transloadit deleted a comment from aduh95 Mar 29, 2023
@mifi
Copy link
Contributor Author

mifi commented Mar 30, 2023

I've reverted the addFile logic now, and tested in dropbox and it works!

One issue with the async generator approach is that we get a waterfall of network requests (everything runs in series, nothing in parallel). This means that listing files will be slower than with pMap (concurrency 10):

Screenshot 2023-03-30 at 14 34 53

...but maybe it's ok, as long as we implement #4388 so the user knows that something is happening. A benefit of waterfall requests is that it is easier on companion (performance wise), and on the underlying api (dropbox/drive etc), so we are less likely to run into throttling from them.

Comment on lines 302 to 307
// Note: this.addFile must be only run once we are done fetching all files,
// because it will cause the loading screen to disappear,
// and that will allow the user to start the upload, so we need to make sure we have
// finished all async operations before we add any file
// see https://github.com/transloadit/uppy/pull/4384
newFiles.forEach((file) => this.addFile(file))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's not something to do in this PR, but we might consider having another way to signal the UI that the loading is not done, as it's not a great UX to get stuck on a loading screen for too long.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, there is currently no deterministic way to know whether we are still adding files. It probably requires as state machine of some sorts, where we can only be in one state at a time and a promise will make it move to the next. But that's a bigger undertaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need to define then what should the UX be like. What do we show the user while files are loading? Is there anything more useful we can do than showing a loading screen and possibly a number of files loaded? We could immediately send them to the list of added files, and then disable the Upload button until everything is added, but I don't know how much value it gives the user to be able to browse files while new files are popping into view continuously, and they will not be able to click Upload.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could immediately send them to the list of added files, and then disable the Upload button until everything is added

To me that with a spinner of some sort or a progress bar is the UX I'd expect. The question is if it's enough of a common use case to prioritize implementing like that, if it's not trivial.

instead of generating an ID, for providers that we whitelsit
this allows adding the same time many times (with a different path)
it seems to be causing problems when dropping folders with subfolders in newest chrome
e.g a folder with 50 files and a subfolder which in turn has another 50 files

also refactor and document the code more
@mifi
Copy link
Contributor Author

mifi commented Mar 31, 2023

have also pushed a fix for folder with subfolders drag-drop not working in newest chrome.
(disable experimental getAsFileSystemHandle with a TODO)

mifi and others added 2 commits April 1, 2023 11:22
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@mifi
Copy link
Contributor Author

mifi commented Apr 1, 2023

I moved the function into utils, now it's failing with another error:

Error: R] No matching export in "node_modules/@uppy/utils/lib/generateFileID.js" for import "getSafeFileId"

But it's clearly there and exported:

export function getSafeFileId (file) {

And it works with yarn dev so i don't know what's going on here

@arturi
Copy link
Contributor

arturi commented Apr 1, 2023

It could be related to export maps added to utils in #3985

"exports": {
"./package.json": "./package.json",
"./lib/Translator": "./lib/Translator.js",
"./lib/EventTracker": "./lib/EventTracker.js",
"./lib/ProgressTimeout": "./lib/ProgressTimeout.js",
"./lib/RateLimitedQueue": "./lib/RateLimitedQueue.js",
"./lib/canvasToBlob": "./lib/canvasToBlob.js",
"./lib/dataURItoBlob": "./lib/dataURItoBlob.js",
"./lib/dataURItoFile": "./lib/dataURItoFile.js",
"./lib/emitSocketProgress": "./lib/emitSocketProgress.js",
"./lib/findAllDOMElements": "./lib/findAllDOMElements.js",
"./lib/findDOMElement": "./lib/findDOMElement.js",
"./lib/generateFileID": "./lib/generateFileID.js",
"./lib/getBytesRemaining": "./lib/getBytesRemaining.js",
"./lib/getETA": "./lib/getETA.js",
"./lib/getFileNameAndExtension": "./lib/getFileNameAndExtension.js",
"./lib/getFileType": "./lib/getFileType.js",
"./lib/getFileTypeExtension": "./lib/getFileTypeExtension.js",
"./lib/getSocketHost": "./lib/getSocketHost.js",
"./lib/getSpeed": "./lib/getSpeed.js",
"./lib/getTimeStamp": "./lib/getTimeStamp.js",
"./lib/isDOMElement": "./lib/isDOMElement.js",
"./lib/isObjectURL": "./lib/isObjectURL.js",
"./lib/isDragDropSupported": "./lib/isDragDropSupported.js",
"./lib/isPreviewSupported": "./lib/isPreviewSupported.js",
"./lib/isTouchDevice": "./lib/isTouchDevice.js",
"./lib/prettyETA": "./lib/prettyETA.js",
"./lib/secondsToTime": "./lib/secondsToTime.js",
"./lib/settle": "./lib/settle.js",
"./lib/toArray": "./lib/toArray.js",
"./lib/FOCUSABLE_ELEMENTS": "./lib/FOCUSABLE_ELEMENTS.js",
"./lib/AbortController": "./lib/AbortController.js",
"./lib/getTextDirection": "./lib/getTextDirection.js",
"./lib/NetworkError": "./lib/NetworkError.js",
"./lib/isNetworkError": "./lib/isNetworkError.js",
"./lib/truncateString": "./lib/truncateString.js",
"./lib/remoteFileObjToLocal": "./lib/remoteFileObjToLocal.js",
"./lib/fetchWithNetworkError": "./lib/fetchWithNetworkError.js",
"./lib/ErrorWithCause": "./lib/ErrorWithCause.js",
"./lib/delay": "./lib/delay.js",
"./lib/hasProperty": "./lib/hasProperty.js",
"./lib/mimeTypes": "./lib/mimeTypes.js",
"./lib/getDroppedFiles": "./lib/getDroppedFiles/index.js",
"./lib/FOCUSABLE_ELEMENTS.js": "./lib/FOCUSABLE_ELEMENTS.js",
"./src/microtip.scss": "./src/microtip.scss"
},

@mifi
Copy link
Contributor Author

mifi commented Apr 2, 2023

I don't know how that works. does it mean I have to use extensions when I import, e.g. @uppy/utils/lib/generateFileID.js? Could we somehow enforce this with eslint or make yarn dev fail also? I would like to get notified about such subtleties while developing

@aduh95
Copy link
Contributor

aduh95 commented Apr 2, 2023

No, after verification that's not it, the file is already in the exports map:

"./lib/generateFileID": "./lib/generateFileID.js",

does it mean I have to use extensions when I import

Quite the contrary, it means we can't use extension, because we only export the path without the extension.

@aduh95
Copy link
Contributor

aduh95 commented Apr 2, 2023

The issue is that the action is using the npm version for 8 of the plugins 🙈 So it fails because the version on npm does not have the export. I'll try to fix the action ASAP. EDIT: I have a working fix in #4395.

@aduh95 aduh95 force-pushed the fix-addfiles-race-condition branch from 2e79bd3 to 46368c8 Compare April 2, 2023 08:59
@arturi
Copy link
Contributor

arturi commented Apr 3, 2023

Shall we merge, @mifi?

@mifi
Copy link
Contributor Author

mifi commented Apr 4, 2023

Ship it!

@mifi mifi merged commit 3dd1e5e into main Apr 4, 2023
@mifi mifi deleted the fix-addfiles-race-condition branch April 4, 2023 03:13
@github-actions github-actions bot mentioned this pull request Apr 4, 2023
github-actions bot added a commit that referenced this pull request Apr 4, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.0.6 | @uppy/status-bar       |   3.1.0 |
| @uppy/aws-s3-multipart |   3.1.3 | @uppy/transloadit      |   3.1.2 |
| @uppy/companion        |   4.4.0 | @uppy/tus              |   3.0.6 |
| @uppy/companion-client |   3.1.2 | @uppy/unsplash         |   3.2.0 |
| @uppy/core             |   3.1.2 | @uppy/url              |   3.3.0 |
| @uppy/dashboard        |   3.3.2 | @uppy/utils            |   5.2.0 |
| @uppy/locales          |   3.1.0 | @uppy/xhr-upload       |   3.1.1 |
| @uppy/provider-views   |   3.2.0 | uppy                   |   3.7.0 |
| @uppy/react            |   3.1.1 |                        |         |

- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus,@uppy/xhr-upload: make sure that we reset serverToken when an upload fails (Mikael Finstad / #4376)
- @uppy/aws-s3-multipart: do not auto-open sockets, clean them up on abort (Antoine du Hamel)
- @uppy/aws-s3: Update types (Minh Hieu / #4294)
- @uppy/companion-client: do not open socket more than once (Artur Paikin)
- @uppy/companion: add `service: 'companion'` to periodic ping (Mikael Finstad / #4383)
- @uppy/companion: add connection keep-alive to dropbox (Mikael Finstad / #4365)
- @uppy/companion: add missing env variable for standalone option (Mikael Finstad / #4382)
- @uppy/companion: add S3 prefix env variable (Mikael Finstad / #4320)
- @uppy/companion: allow local ips when testing (Mikael Finstad / #4328)
- @uppy/companion: fix typo in redis-emitter.js (Ikko Eltociear Ashimine / #4362)
- @uppy/companion: merge Provider/SearchProvider (Mikael Finstad / #4330)
- @uppy/companion: only body parse when needed & increased body size for s3 (Mikael Finstad / #4372)
- @uppy/core: fix bug with `setOptions` (Nguyễn bảo Trung / #4350)
- @uppy/locales: locales: add es_MX (Kevin van Zonneveld / #4393)
- @uppy/locales: locales: add hi_IN (Kevin van Zonneveld / #4391)
- @uppy/provider-views: fix race condition when adding folders (Mikael Finstad / #4384)
- @uppy/provider-views: UI: Use form attribite with a form in doc root to prevent outer form submit (Artur Paikin / #4283)
- @uppy/transloadit: fix socket error message (Artur Paikin / #4352)
- @uppy/tus: do not auto-open sockets, clean them up on abort (Antoine du Hamel)
- meta: add version info in the bundlers CI (Antoine du Hamel / #4386)
- meta: deploy to Heroku on every companion commit (Mikael Finstad / #4367)
- meta: example: migrate `redux` to ESM (Antoine du Hamel / #4158)
- meta: fix all ESLint warnings and turn them into errors (Antoine du Hamel / #4398)
- meta: fixup! website: update links to work under the new URL (Antoine du Hamel / #4371)
- meta: remove duplicate outdated OSS support docs (Mikael Finstad, Artur Paikin / #4364)
- meta: use overrides to make sure no uppy package is fetch from npm (Antoine du Hamel / #4395)
- website: add a deprecation notice and a link to the new website (Antoine du Hamel / #4370)
- website: fix home page (Antoine du Hamel)
- website: Remove the website (Merlijn Vos / #4369)
- website: update links to work under the new URL (Antoine du Hamel / #4371)
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.0.6 | @uppy/status-bar       |   3.1.0 |
| @uppy/aws-s3-multipart |   3.1.3 | @uppy/transloadit      |   3.1.2 |
| @uppy/companion        |   4.4.0 | @uppy/tus              |   3.0.6 |
| @uppy/companion-client |   3.1.2 | @uppy/unsplash         |   3.2.0 |
| @uppy/core             |   3.1.2 | @uppy/url              |   3.3.0 |
| @uppy/dashboard        |   3.3.2 | @uppy/utils            |   5.2.0 |
| @uppy/locales          |   3.1.0 | @uppy/xhr-upload       |   3.1.1 |
| @uppy/provider-views   |   3.2.0 | uppy                   |   3.7.0 |
| @uppy/react            |   3.1.1 |                        |         |

- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus,@uppy/xhr-upload: make sure that we reset serverToken when an upload fails (Mikael Finstad / transloadit#4376)
- @uppy/aws-s3-multipart: do not auto-open sockets, clean them up on abort (Antoine du Hamel)
- @uppy/aws-s3: Update types (Minh Hieu / transloadit#4294)
- @uppy/companion-client: do not open socket more than once (Artur Paikin)
- @uppy/companion: add `service: 'companion'` to periodic ping (Mikael Finstad / transloadit#4383)
- @uppy/companion: add connection keep-alive to dropbox (Mikael Finstad / transloadit#4365)
- @uppy/companion: add missing env variable for standalone option (Mikael Finstad / transloadit#4382)
- @uppy/companion: add S3 prefix env variable (Mikael Finstad / transloadit#4320)
- @uppy/companion: allow local ips when testing (Mikael Finstad / transloadit#4328)
- @uppy/companion: fix typo in redis-emitter.js (Ikko Eltociear Ashimine / transloadit#4362)
- @uppy/companion: merge Provider/SearchProvider (Mikael Finstad / transloadit#4330)
- @uppy/companion: only body parse when needed & increased body size for s3 (Mikael Finstad / transloadit#4372)
- @uppy/core: fix bug with `setOptions` (Nguyễn bảo Trung / transloadit#4350)
- @uppy/locales: locales: add es_MX (Kevin van Zonneveld / transloadit#4393)
- @uppy/locales: locales: add hi_IN (Kevin van Zonneveld / transloadit#4391)
- @uppy/provider-views: fix race condition when adding folders (Mikael Finstad / transloadit#4384)
- @uppy/provider-views: UI: Use form attribite with a form in doc root to prevent outer form submit (Artur Paikin / transloadit#4283)
- @uppy/transloadit: fix socket error message (Artur Paikin / transloadit#4352)
- @uppy/tus: do not auto-open sockets, clean them up on abort (Antoine du Hamel)
- meta: add version info in the bundlers CI (Antoine du Hamel / transloadit#4386)
- meta: deploy to Heroku on every companion commit (Mikael Finstad / transloadit#4367)
- meta: example: migrate `redux` to ESM (Antoine du Hamel / transloadit#4158)
- meta: fix all ESLint warnings and turn them into errors (Antoine du Hamel / transloadit#4398)
- meta: fixup! website: update links to work under the new URL (Antoine du Hamel / transloadit#4371)
- meta: remove duplicate outdated OSS support docs (Mikael Finstad, Artur Paikin / transloadit#4364)
- meta: use overrides to make sure no uppy package is fetch from npm (Antoine du Hamel / transloadit#4395)
- website: add a deprecation notice and a link to the new website (Antoine du Hamel / transloadit#4370)
- website: fix home page (Antoine du Hamel)
- website: Remove the website (Merlijn Vos / transloadit#4369)
- website: update links to work under the new URL (Antoine du Hamel / transloadit#4371)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants