Skip to content

Commit

Permalink
@uppy/provider-views: fix race conditions with folder loading (#4578)
Browse files Browse the repository at this point in the history 
* fix multiple race conditions

multiple race conditions / bugs:
- If you open a folder in e.g. facebook and you scroll down, triggering the infinity scroll, and then go back (one folder up) before the infinity scroll has finished loading, you could end up with the contents of the scrolled folder instead of the previous folder
- if you select a folder structure (e.g. deep structure with many files) and then click "cancel" while loading, it will still continue loading recursively in the background even after cancelled and potentially cause more issues down the road

* Apply suggestions from code review

Co-authored-by: Antoine du Hamel <antoine@transloadit.com>

* merge conditions

---------

Co-authored-by: Antoine du Hamel <antoine@transloadit.com>
Co-authored-by: Murderlon <merlijn@soverin.net>
  • Loading branch information
@mifi @aduh95 @Murderlon
3 people authored Jul 20, 2023
1 parent 49cf48d commit ecdecd1
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 120 deletions.
4 changes: 2 additions & 2 deletions packages/@uppy/companion-client/src/Provider.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ export default class Provider extends RequestClient {
return this.get(`${this.id}/list/${directory || ''}`, options)
}

async logout () {
const response = await this.get(`${this.id}/logout`)
async logout (options) {
const response = await this.get(`${this.id}/logout`, options)
await this.#removeAuthToken()
return response
}
Expand Down
256 changes: 139 additions & 117 deletions packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,29 @@ export default class ProviderView extends View {
// Nothing.
}

#abortController

async #withAbort (op) {
// prevent multiple requests in parallel from causing race conditions
this.#abortController?.abort()
const abortController = new AbortController()
this.#abortController = abortController
const cancelRequest = () => {
abortController.abort()
this.clearSelection()
}
try {
this.plugin.uppy.on('dashboard:close-panel', cancelRequest)
this.plugin.uppy.on('cancel-all', cancelRequest)

await op(abortController.signal)
} finally {
this.plugin.uppy.off('dashboard:close-panel', cancelRequest)
this.plugin.uppy.off('cancel-all', cancelRequest)
this.#abortController = undefined
}
}

async #list ({ requestPath, absDirPath, signal }) {
const { username, nextPagePath, items } = await this.provider.list(requestPath, { signal })
this.username = username || this.username
Expand Down Expand Up @@ -139,58 +162,45 @@ export default class ProviderView extends View {
* @returns {Promise} Folders/files in folder
*/
async getFolder (requestPath, name) {
const controller = new AbortController()
const cancelRequest = () => {
controller.abort()
this.clearSelection()
}

this.plugin.uppy.on('dashboard:close-panel', cancelRequest)
this.plugin.uppy.on('cancel-all', cancelRequest)

this.setLoading(true)
try {
this.lastCheckbox = undefined
await this.#withAbort(async (signal) => {
this.lastCheckbox = undefined

let { breadcrumbs } = this.plugin.getPluginState()
let { breadcrumbs } = this.plugin.getPluginState()

const index = breadcrumbs.findIndex((dir) => requestPath === dir.requestPath)
const index = breadcrumbs.findIndex((dir) => requestPath === dir.requestPath)

if (index !== -1) {
// means we navigated back to a known directory (already in the stack), so cut the stack off there
breadcrumbs = breadcrumbs.slice(0, index + 1)
} else {
// we have navigated into a new (unknown) folder, add it to the stack
breadcrumbs = [...breadcrumbs, { requestPath, name }]
}
if (index !== -1) {
// means we navigated back to a known directory (already in the stack), so cut the stack off there
breadcrumbs = breadcrumbs.slice(0, index + 1)
} else {
// we have navigated into a new (unknown) folder, add it to the stack
breadcrumbs = [...breadcrumbs, { requestPath, name }]
}

this.nextPagePath = requestPath
let files = []
let folders = []
do {
const { files: newFiles, folders: newFolders } = await this.#listFilesAndFolders({
breadcrumbs, signal: controller.signal,
})
this.nextPagePath = requestPath
let files = []
let folders = []
do {
const { files: newFiles, folders: newFolders } = await this.#listFilesAndFolders({
breadcrumbs, signal,
})

files = files.concat(newFiles)
folders = folders.concat(newFolders)
files = files.concat(newFiles)
folders = folders.concat(newFolders)

this.setLoading(this.plugin.uppy.i18n('loadedXFiles', { numFiles: files.length + folders.length }))
} while (
this.opts.loadAllFiles && this.nextPagePath
)
this.setLoading(this.plugin.uppy.i18n('loadedXFiles', { numFiles: files.length + folders.length }))
} while (
this.opts.loadAllFiles && this.nextPagePath
)

this.plugin.setPluginState({ folders, files, breadcrumbs, filterInput: '' })
this.plugin.setPluginState({ folders, files, breadcrumbs, filterInput: '' })
})
} catch (err) {
if (err.cause?.name === 'AbortError') {
// Expected, user clicked “cancel”
return
}
this.handleError(err)
} finally {
this.setLoading(false)
this.plugin.uppy.off('dashboard:close-panel', cancelRequest)
this.plugin.uppy.off('cancel-all', cancelRequest)
}
}

Expand All @@ -207,9 +217,10 @@ export default class ProviderView extends View {
/**
* Removes session token on client side.
*/
logout () {
this.provider.logout()
.then((res) => {
async logout () {
try {
await this.#withAbort(async (signal) => {
const res = await this.provider.logout({ signal })
if (res.ok) {
if (!res.revoked) {
const message = this.plugin.uppy.i18n('companionUnauthorizeHint', {
Expand All @@ -228,7 +239,10 @@ export default class ProviderView extends View {
}
this.plugin.setPluginState(newState)
}
}).catch(this.handleError)
})
} catch (err) {
this.handleError(err)
}
}

filterQuery (input) {
Expand Down Expand Up @@ -286,14 +300,18 @@ export default class ProviderView extends View {
this.isHandlingScroll = true

try {
const { files, folders, breadcrumbs } = this.plugin.getPluginState()
await this.#withAbort(async (signal) => {
const { files, folders, breadcrumbs } = this.plugin.getPluginState()

const { files: newFiles, folders: newFolders } = await this.#listFilesAndFolders({ breadcrumbs })
const { files: newFiles, folders: newFolders } = await this.#listFilesAndFolders({
breadcrumbs, signal,
})

const combinedFiles = files.concat(newFiles)
const combinedFolders = folders.concat(newFolders)
const combinedFiles = files.concat(newFiles)
const combinedFolders = folders.concat(newFolders)

this.plugin.setPluginState({ folders: combinedFolders, files: combinedFiles })
this.plugin.setPluginState({ folders: combinedFolders, files: combinedFiles })
})
} catch (error) {
this.handleError(error)
} finally {
Expand All @@ -302,11 +320,11 @@ export default class ProviderView extends View {
}
}

async #recursivelyListAllFiles ({ requestPath, absDirPath, relDirPath, queue, onFiles }) {
async #recursivelyListAllFiles ({ requestPath, absDirPath, relDirPath, queue, onFiles, signal }) {
let curPath = requestPath

while (curPath) {
const res = await this.#list({ requestPath: curPath, absDirPath })
const res = await this.#list({ requestPath: curPath, absDirPath, signal })
curPath = res.nextPagePath

const files = res.items.filter((item) => !item.isFolder)
Expand All @@ -322,6 +340,7 @@ export default class ProviderView extends View {
relDirPath: prependPath(relDirPath, folder.name),
queue,
onFiles,
signal,
})
)))
await Promise.all(promises) // in case we get an error
Expand All @@ -331,87 +350,90 @@ export default class ProviderView extends View {
async donePicking () {
this.setLoading(true)
try {
const { currentSelection } = this.plugin.getPluginState()
await this.#withAbort(async (signal) => {
const { currentSelection } = this.plugin.getPluginState()

const messages = []
const newFiles = []
const messages = []
const newFiles = []

for (const selectedItem of currentSelection) {
const { requestPath } = selectedItem
for (const selectedItem of currentSelection) {
const { requestPath } = selectedItem

const withRelDirPath = (newItem) => ({
...newItem,
// calculate the file's path relative to the user's selected item's path
// see https://github.com/transloadit/uppy/pull/4537#issuecomment-1614236655
relDirPath: newItem.absDirPath.replace(selectedItem.absDirPath, '').replace(/^\//, ''),
})
const withRelDirPath = (newItem) => ({
...newItem,
// calculate the file's path relative to the user's selected item's path
// see https://github.com/transloadit/uppy/pull/4537#issuecomment-1614236655
relDirPath: newItem.absDirPath.replace(selectedItem.absDirPath, '').replace(/^\//, ''),
})

if (selectedItem.isFolder) {
let isEmpty = true
let numNewFiles = 0

const queue = new PQueue({ concurrency: 6 })

const onFiles = (files) => {
for (const newFile of files) {
const tagFile = this.getTagFile(newFile)
const id = getSafeFileId(tagFile)
// If the same folder is added again, we don't want to send
// X amount of duplicate file notifications, we want to say
// the folder was already added. This checks if all files are duplicate,
// if that's the case, we don't add the files.
if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) {
newFiles.push(withRelDirPath(newFile))
numNewFiles++
this.setLoading(this.plugin.uppy.i18n('addedNumFiles', { numFiles: numNewFiles }))
if (selectedItem.isFolder) {
let isEmpty = true
let numNewFiles = 0

const queue = new PQueue({ concurrency: 6 })

const onFiles = (files) => {
for (const newFile of files) {
const tagFile = this.getTagFile(newFile)
const id = getSafeFileId(tagFile)
// If the same folder is added again, we don't want to send
// X amount of duplicate file notifications, we want to say
// the folder was already added. This checks if all files are duplicate,
// if that's the case, we don't add the files.
if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) {
newFiles.push(withRelDirPath(newFile))
numNewFiles++
this.setLoading(this.plugin.uppy.i18n('addedNumFiles', { numFiles: numNewFiles }))
}
isEmpty = false
}
isEmpty = false
}
}

await this.#recursivelyListAllFiles({
requestPath,
absDirPath: prependPath(selectedItem.absDirPath, selectedItem.name),
relDirPath: selectedItem.name,
queue,
onFiles,
})
await queue.onIdle()

let message
if (isEmpty) {
message = this.plugin.uppy.i18n('emptyFolderAdded')
} else if (numNewFiles === 0) {
message = this.plugin.uppy.i18n('folderAlreadyAdded', {
folder: selectedItem.name,
await this.#recursivelyListAllFiles({
requestPath,
absDirPath: prependPath(selectedItem.absDirPath, selectedItem.name),
relDirPath: selectedItem.name,
queue,
onFiles,
signal,
})
await queue.onIdle()

let message
if (isEmpty) {
message = this.plugin.uppy.i18n('emptyFolderAdded')
} else if (numNewFiles === 0) {
message = this.plugin.uppy.i18n('folderAlreadyAdded', {
folder: selectedItem.name,
})
} else {
// TODO we don't really know at this point whether any files were actually added
// (only later after addFiles has been called) so we should probably rewrite this.
// Example: If all files fail to add due to restriction error, it will still say "Added 100 files from folder"
message = this.plugin.uppy.i18n('folderAdded', {
smart_count: numNewFiles, folder: selectedItem.name,
})
}

messages.push(message)
} else {
// TODO we don't really know at this point whether any files were actually added
// (only later after addFiles has been called) so we should probably rewrite this.
// Example: If all files fail to add due to restriction error, it will still say "Added 100 files from folder"
message = this.plugin.uppy.i18n('folderAdded', {
smart_count: numNewFiles, folder: selectedItem.name,
})
newFiles.push(withRelDirPath(selectedItem))
}

messages.push(message)
} else {
newFiles.push(withRelDirPath(selectedItem))
}
}

// Note: this.plugin.uppy.addFiles 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
this.plugin.uppy.log('Adding remote provider files')
this.plugin.uppy.addFiles(newFiles.map((file) => this.getTagFile(file)))
// Note: this.plugin.uppy.addFiles 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
this.plugin.uppy.log('Adding remote provider files')
this.plugin.uppy.addFiles(newFiles.map((file) => this.getTagFile(file)))

this.plugin.setPluginState({ filterInput: '' })
messages.forEach(message => this.plugin.uppy.info(message))
this.plugin.setPluginState({ filterInput: '' })
messages.forEach(message => this.plugin.uppy.info(message))

this.clearSelection()
this.clearSelection()
})
} catch (err) {
this.handleError(err)
} finally {
Expand Down
4 changes: 3 additions & 1 deletion packages/@uppy/provider-views/src/View.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ export default class View {

uppy.log(error.toString())

if (error.isAuthError) {
if (error.isAuthError || error.cause?.name === 'AbortError') {
// authError just means we're not authenticated, don't show to user
// AbortError means the user has clicked "cancel" on an operation
return
}

Expand Down

0 comments on commit ecdecd1

Please sign in to comment.