Rewrite Companion providers to use streams to allow simultaneous upload/download without saving to disk #3159

mifi · 2021-09-02T07:29:20Z

Make upload start almost immediately without having to first download the file. Allows for uploading bigger files, because transloadit assembly will no longer timeout, as it will get upload progress events all the time. No longer need for illusive progress.

This also removes the need for disk space as data will be buffered in memory and back-pressure will be respected automatically. #3098 (comment)
All providers "download" methods will now return a { stream } which will be consumed by uploader.

Also:

Change Provider/SearchProvider API to async (Breaking change for custom companion providers)
rewrite controllers deauth-callback, thumbnail, list, logout to async
fix purest mock (it wasn't returning statusCode on('response'))
fix a bug where remote xhr upload would ignore progress events in the UI and never show uploaded state after upload finishes
fix bug where s3 multipart cancel wasn't working
fix bug where xhr cancel wasn't working
getURLMeta: make sure size is never NaN (NaN gets converted to null in JSON.stringify when sent to client but not when used in backend)
add missing http mock for "request" for THUMBNAIL_URL and http://url.myendpoint.com/file (these request errors were never caught by tests previously)
"upload functions with tus protocol" test: move filename checking to new test where size is null. Fix broken expects
improve dev Dashboard a bit
simplify logic in Uploader.js.
fix some lint

TODO:

unit test hangs due to jest having issue with dynamic import of an ESM module (await import('fs-capacitor')) - fixed by moving away from fs-capacitor
make simultaneous dl/ul behind a flag (COMPANION_STREAMING_UPLOAD/streamingUpload), default to false
add an option for limiting file size (COMPANION_MAX_FILE_SIZE / maxFileSize)
implement progress events for download too (unknown length streams or when streamingUpload=false)
Update docs for custom provider https://uppy.io/docs/companion/#Adding-custom-providers (have updated example only)
try to make backward compatibility layer for providers
pause/resume tus upload no longer works properly, due to this issue which must be fixed before merge: abort() followed by start() is broken on non-file streams tus/tus-js-client#275 - though I believe we can still merge this PR without it because streamingUpload defaults to false
need some more testing (dropbox, instagram, onedrive, zoom), maybe test once deployed to uppy.io?

Future improvements:

for TUS uploads (and I believe form upload), we don't need to download file first if size is unknown. I think those can both handle unknown length streams
cancel upload also when canceling download (readStream). now it will just eventually time out due to no more data from the readstream

Note: This PR is actually a fork off from from branch companion-optional-head (PR #3048) so includes those commits.

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

and abort request immediately upon response headers received #3034 (comment)

or else we get "MetadataTooLarge: Your metadata headers exceed the maximum allowed metadata size" in tus / S3

This allows for upload to start almost immediately without having to first download the file. And it allows for uploading bigger files, because transloadit assembly will not timeout, as it will get upload progress events all the time. No longer need for illusive progress. Also fix eslint warnings and simplify logic Still TODO: TUS pause/resume has a bug: tus/tus-js-client#275

This removes the need for disk space as data will be buffered in memory and backpressure will be respected #3098 (comment) All providers "download" methods will now return a { stream } which can be consumed by uploader. Also: - Remove capacitor (no longer needed) - Change Provider/SearchProvider API to async (Breaking change for custom companion providers) - Fix the case with unknown length streams (zoom / google drive). Need to be downloaded first - rewrite controllers deauth-callback, thumbnail, list, logout to async - getURLMeta: make sure size is never NaN (NaN gets converted to null in JSON.stringify when sent to client but not when used in backend) - fix purest mock (it wasn't returning statusCode on('response')) - add missing http mock for "request" for THUMBNAIL_URL and http://url.myendpoint.com/file (these request errors were never caught by tests previously) - "upload functions with tus protocol" test: move filename checking to new test where size is null. Fix broken expects - fix some lint

COMPANION_STREAMING_UPLOAD Default to false due to backward compatibility If set to true, will start to upload files at the same time as dowlnoading them, by piping the streams - Also implement progress for downloading too - and fix progress duplication logic - fix test that assumed file was fully downloaded after first progress event

for both unknown length and known length downloads

streams were being kept

packages/@uppy/companion/src/server/Uploader.js

aduh95 · 2021-10-23T16:28:44Z

packages/@uppy/companion/src/server/Uploader.js

+  }
+
+  async _downloadStreamAsFile (stream) {
+    this.tmpPath = join(this.options.pathPrefix, this.fileName)


We should consider adding support for URL objects (doesn't have to be in this PR though).

packages/@uppy/companion/src/server/Uploader.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Murderlon

Impressive PR with a valuable feature :)

packages/@uppy/companion/src/server/provider/unsplash/index.js

website/src/docs/companion.md

Co-authored-by: Merlijn Vos <merlijn@soverin.net>

aduh95

I think we should try to optimize away those promisify+.bind calls in the providers.

packages/@uppy/companion/src/server/Uploader.js

packages/@uppy/companion/src/server/controllers/url.js

packages/@uppy/companion/src/server/helpers/request.js

packages/@uppy/companion/src/server/helpers/upload.js

packages/@uppy/companion/src/server/provider/box/index.js

packages/@uppy/companion/src/server/provider/drive/index.js

…n-upload-while-downloading

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

#3159 (comment)

packages/@uppy/companion/test/__tests__/uploader.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

* main: Create docs for `@uppy/screen-capture` (#3282) Fix bug introduced in #3159 rename master to main Rewrite Companion providers to use streams to allow simultaneous upload/download without saving to disk (#3159) chore: Add `FileRemoveReason` type (#3283) ci: test with Node.js 17.x (#3274) meta: use `workspace:^` in peerDependencies (#3278) meta: use `workspace:^` instead of `workspace:*` (#3278) meta: upgrade Yarn to 3.1.0 (#3278) meta: add `update-yarn` script (#3278)

those are the only two providers who use companion-proxied thumbnails dropbox had been broken after #3159 box did never work before because it just showed a placeholder (see box api) also upgraded dropbox thumbnail api to v2 and rewrite to async/await

…ad/download without saving to disk (transloadit#3159) * rewrite to async/await * Only fetch size (HEAD) if needed transloadit#3034 * Update packages/@uppy/companion/src/server/controllers/url.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * Change HEAD to GET in getURLMeta and abort request immediately upon response headers received transloadit#3034 (comment) * fix lint * fix lint * cut off length of file names or else we get "MetadataTooLarge: Your metadata headers exceed the maximum allowed metadata size" in tus / S3 * try to fix flaky test * remove iife and cleanup code a bit * fix lint by reordering code * rename Uploader to MultipartUploader * Rewrite Uploader to use fs-capacitor transloadit#3098 This allows for upload to start almost immediately without having to first download the file. And it allows for uploading bigger files, because transloadit assembly will not timeout, as it will get upload progress events all the time. No longer need for illusive progress. Also fix eslint warnings and simplify logic Still TODO: TUS pause/resume has a bug: tus/tus-js-client#275 * add comment in dev Dashboard and pull out variable * fix a bug where remote xhr upload would ignore progress events in the UI * fix bug where s3 multipart cancel wasn't working * fix also cancel for xhr * Rewrite providers to use streams This removes the need for disk space as data will be buffered in memory and backpressure will be respected transloadit#3098 (comment) All providers "download" methods will now return a { stream } which can be consumed by uploader. Also: - Remove capacitor (no longer needed) - Change Provider/SearchProvider API to async (Breaking change for custom companion providers) - Fix the case with unknown length streams (zoom / google drive). Need to be downloaded first - rewrite controllers deauth-callback, thumbnail, list, logout to async - getURLMeta: make sure size is never NaN (NaN gets converted to null in JSON.stringify when sent to client but not when used in backend) - fix purest mock (it wasn't returning statusCode on('response')) - add missing http mock for "request" for THUMBNAIL_URL and http://url.myendpoint.com/file (these request errors were never caught by tests previously) - "upload functions with tus protocol" test: move filename checking to new test where size is null. Fix broken expects - fix some lint * Implement streamingUpload flag COMPANION_STREAMING_UPLOAD Default to false due to backward compatibility If set to true, will start to upload files at the same time as dowlnoading them, by piping the streams - Also implement progress for downloading too - and fix progress duplication logic - fix test that assumed file was fully downloaded after first progress event * rearrange validation logic * add COMPANION_STREAMING_UPLOAD to env.test.sh too * implement maxFileSize option in companion for both unknown length and known length downloads * fix bug * fix memory leak when non 200 status streams were being kept * fix lint * Add backward-compatibility for companion providers Implement a new static field "version" on providers, which when not set to 2, will cause a compatibility layer to be added for supporting old callback style provider api also fix some eslint and rename some vars * document new provider API * remove static as it doesn't work on node 10 * try to fix build issue * degrade to node 14 in github actions due to hitting this error: nodejs/node#40030 https://github.com/transloadit/uppy/pull/3159/checks?check_run_id=3544858518 * pull out duplicated logic into reusable function * fix lint * make methods private * re-add unsplash download_location request got lost in merge * add try/catch as suggested transloadit#3159 (comment) * Only set default chunkSize if needed for being more compliant with previous behavior when streamingUpload = false * Improve flaky test Trying to fix this error: FAIL packages/@uppy/utils/src/delay.test.js ● delay › should reject when signal is aborted expect(received).toBeLessThan(expected) Expected: < 70 Received: 107 32 | const time = Date.now() - start 33 | expect(time).toBeGreaterThanOrEqual(30) > 34 | expect(time).toBeLessThan(70) | ^ 35 | }) 36 | }) 37 | at Object.<anonymous> (packages/@uppy/utils/src/delay.test.js:34:18) https://github.com/transloadit/uppy/runs/3984613454?check_suite_focus=true * Apply suggestions from code review Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * fix review feedback & lint * Apply suggestions from code review Co-authored-by: Merlijn Vos <merlijn@soverin.net> * remove unneeded ts-ignore * Update packages/@uppy/companion/src/server/controllers/url.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * Update packages/@uppy/companion/src/server/Uploader.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * reduce nesting * fix lint * optimize promisify transloadit#3159 (comment) * Update packages/@uppy/companion/test/__tests__/uploader.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Merlijn Vos <merlijn@soverin.net>

those are the only two providers who use companion-proxied thumbnails dropbox had been broken after transloadit#3159 box did never work before because it just showed a placeholder (see box api) also upgraded dropbox thumbnail api to v2 and rewrite to async/await

mifi and others added 16 commits July 26, 2021 17:10

rewrite to async/await

27c64c7

Only fetch size (HEAD) if needed #3034

ce4e8f3

Update packages/@uppy/companion/src/server/controllers/url.js

b9d2091

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Merge branch 'main' into companion-optional-head

88b7c51

Change HEAD to GET in getURLMeta

0b6c488

and abort request immediately upon response headers received #3034 (comment)

fix lint

77c4f25

fix lint

4226c95

cut off length of file names

cf494e5

or else we get "MetadataTooLarge: Your metadata headers exceed the maximum allowed metadata size" in tus / S3

try to fix flaky test

4aabeea

remove iife and cleanup code a bit

11429a7

fix lint by reordering code

0ba8349

rename Uploader to MultipartUploader

55370c5

add comment in dev Dashboard and pull out variable

7a08af4

fix a bug where remote xhr upload would ignore progress events in the UI

2d6efdf

fix bug where s3 multipart cancel wasn't working

1a77274

Murderlon marked this pull request as draft September 2, 2021 08:54

Murderlon changed the title ~~WIP: Companion upload/downloading simultaneously using fs-capacitor~~ Companion upload/downloading simultaneously using fs-capacitor Sep 2, 2021

mifi added 2 commits September 2, 2021 18:36

fix also cancel for xhr

1b763be

mifi changed the title ~~Companion upload/downloading simultaneously using fs-capacitor~~ Rewrite Companion providers to use streams to allow simultaneous upload/download without saving to disk Sep 3, 2021

Merge branch 'main' into companion-upload-while-downloading

2f7fc2d

This was referenced Sep 3, 2021

Uploading from Google Drive hangs after a while #3098

Closed

Companion downloads/uploads never canceled #3155

Closed

mifi added 6 commits September 7, 2021 15:53

rearrange validation logic

bd255aa

add COMPANION_STREAMING_UPLOAD to env.test.sh too

e756b0a

implement maxFileSize option in companion

9113472

for both unknown length and known length downloads

fix bug

919ec18

fix memory leak when non 200 status

eecdba9

streams were being kept

aduh95 reviewed Oct 23, 2021

View reviewed changes

mifi and others added 2 commits October 24, 2021 21:11

Apply suggestions from code review

d0ff2cc

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

fix review feedback & lint

9a6af79

mifi requested a review from aduh95 October 27, 2021 07:52

Murderlon approved these changes Oct 27, 2021

View reviewed changes

packages/@uppy/companion/src/server/provider/unsplash/index.js Outdated Show resolved Hide resolved

website/src/docs/companion.md Outdated Show resolved Hide resolved

Apply suggestions from code review

4eff356

Co-authored-by: Merlijn Vos <merlijn@soverin.net>

aduh95 reviewed Oct 28, 2021

View reviewed changes

mifi and others added 7 commits October 28, 2021 21:57

Merge commit 'e30601df8a89c1a83a6fdb2eb1c8e0eea5cd42cf' into companio…

0f148b5

…n-upload-while-downloading

remove unneeded ts-ignore

ecb86da

Update packages/@uppy/companion/src/server/controllers/url.js

b775d4a

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Update packages/@uppy/companion/src/server/Uploader.js

259e493

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

reduce nesting

21b9d64

fix lint

2338d2f

optimize promisify

55a754a

#3159 (comment)

mifi requested a review from aduh95 October 28, 2021 15:54

aduh95 approved these changes Oct 29, 2021

View reviewed changes

packages/@uppy/companion/test/__tests__/uploader.js Outdated Show resolved Hide resolved

Update packages/@uppy/companion/test/__tests__/uploader.js

291d8e3

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

mifi mentioned this pull request Nov 1, 2021

Companion improvements #3284

Open

11 tasks

mifi merged commit 56339fc into main Nov 1, 2021

mifi deleted the companion-upload-while-downloading branch November 1, 2021 09:35

mifi added a commit that referenced this pull request Nov 1, 2021

Fix bug introduced in #3159

639988f

mifi mentioned this pull request Nov 3, 2021

100GB file from google drive #3295

Closed

mifi mentioned this pull request Dec 16, 2021

website: 2.0-2.3 post draft #3370

Merged

mifi mentioned this pull request Feb 1, 2022

fix broken thumbnails for box and dropbox #3460

Merged

HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023

Fix bug introduced in transloadit#3159

0dc288a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Companion providers to use streams to allow simultaneous upload/download without saving to disk #3159

Rewrite Companion providers to use streams to allow simultaneous upload/download without saving to disk #3159

mifi commented Sep 2, 2021 •

edited

aduh95 Oct 23, 2021

Murderlon left a comment

aduh95 left a comment

Rewrite Companion providers to use streams to allow simultaneous upload/download without saving to disk #3159

Rewrite Companion providers to use streams to allow simultaneous upload/download without saving to disk #3159

Conversation

mifi commented Sep 2, 2021 • edited

aduh95 Oct 23, 2021

Choose a reason for hiding this comment

Murderlon left a comment

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

mifi commented Sep 2, 2021 •

edited