Refresh file before calling user-defined functions in AWS S3 Multipart #4557
Conversation
maddy-jo
force-pushed
the
aws-s3-multipart-metadata
branch
from
076898f to
af6e431
Compare
| await core.upload() | ||
| expect(createMultipartUpload).toHaveBeenCalled() | ||
| expect(signPartWithAbort).toHaveBeenCalled() | ||
| expect(abortMultipartUpload).toHaveBeenCalled() |
There was a problem hiding this comment.
I'm getting the following test failure:
FAIL packages/@uppy/aws-s3-multipart/src/index.test.js
● AwsS3Multipart › file metadata across custom main functions › preserves file metadata if upload is aborted
expect(jest.fn()).toHaveBeenCalled()
Expected number of calls: >= 1
Received number of calls: 0
There was a problem hiding this comment.
hm, okay. that test passes for me, so maybe it's not deterministic as written? also, just to confirm, it's abortMultipartUpload that's not getting called?
I'll take a look at the code and see if I can figure out why that might be failing.
in the meantime, if you have any thoughts on asynchronous logic that might be causing a race condition, please let me know; if not, that's fine too. also, is core.removeFile(file.id) an appropriate way to trigger that abort?
There was a problem hiding this comment.
abortMultipartUploadthat's not getting called?
Yes correct, the failure points to that line, and commenting it does make the test pass.
core.removeFile(file.id)an appropriate way to trigger that abort?
Yes I'd say so.
There was a problem hiding this comment.
@aduh95 can you share any more information about the environment the tests should run in? I'm wondering if a brief timeout or a setTimeout(cb, 0) to clear the event loop would address that failure, but it's hard to know without being able to reproduce it.
There was a problem hiding this comment.
I find it surprising that you are not able to reproduce it, could you share your workflow for running the tests maybe? Here's how I do it:
corepack yarn install
corepack yarn build
corepack yarn test:unitcan you share any more information about the environment the tests should run in?
Since CI is seeing the same failure as I do locally, maybe comparing your environment with the CI will be easier to find what the discrepancy might be?
There was a problem hiding this comment.
okay, was able to reproduce it. not quite sure exactly why it wasn't happening before, but setting my Node version to 18, using corepack (I think not using corepack may have been the issue?), and rebasing against the upstream repo gave me the test failure.
anyway, the specific issue was that #setS3MultipartState was being called after the file was removed from state in Uppy.removeFiles. since that call threw an error, abortFileUpload never reached the part where it called the custom function. adding a null/undefined check for that file when refreshed from the store now allows the pass to test. I'm somewhat surprised that this issue came up with this change, but hoping that this fix here is the appropriate way to resolve it.
I'll watch the PR for any new CI issues and address them as they come up.
maddy-jo
force-pushed
the
aws-s3-multipart-metadata
branch
from
af6e431 to
8f3ef8e
Compare
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3-multipart | 3.5.2 | uppy | 3.13.1 | | @uppy/utils | 5.4.3 | | | - @uppy/utils: align version of `preact` with the UI plugins (Antoine du Hamel / #4599) - @uppy/aws-s3-multipart: refresh file before calling user-defined functions (mjlumetta / #4557) - @uppy/utils: align version of `preact` with the UI plugins (Antoine du Hamel / #4599)
* main: @uppy/aws-s3-multipart: refresh file before calling user-defined functions (#4557) Release: uppy@3.13.0 (#4595) Add i18n to CONTRIBUTING.md (#4591) Add VirtualList to ProviderView (#4566) @uppy/provider-views: fix race conditions with folder loading (#4578) @uppy/status-bar: fix ETA when status bar is installed during upload (#4588) @uppy/provider-views: fix infinite folder loading (#4590) examples/aws: client-side signing (#4463) Bump word-wrap from 1.2.3 to 1.2.4 (#4586) e2e: increase `requestTimeout` to 16s (#4587) @uppy/locales: update zh_TW translation (#4583) @uppy/aws-s3-multipart: fix crash on pause/resume (#4581) @uppy/aws-s3-multipart: do not access `globalThis.crypto` on the top-level (#4584) Release: uppy@3.12.0 (#4574) @uppy/transloadit: fix error message (#4572) @uppy/provider-views: add support for remote file paths (#4537) @uppy/transloadit: implement Server-sent event API (#4098)
Resolves File objects passed to AWS S3 Multipart contains outdated state information. Now the
HTTPCommunicationQueuein the AWS S3 Multipart plugin has a private#getFilefunction, passed when the communication queue is instantiated, which is called to refresh the file object when calling the following user-defined functions:createMultipartUploadsignPartlistPartsabortMultipartUploadcompleteMultipartUploadSee also the issue on the community forum in which I initially raised this issue.
Please let me know if you have any feedback. In particular, I wasn't sure how to design the unit tests, but hope that they're usable based on the pattern of other tests.