googleapis / googleapis/google-cloud-node
TransferManager uploadFileInChunks does not error when aborted
- Dominant language
- TypeScript
- Stars
- 3.2k
- Forks
- 712
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 99
Description
### Please make sure you have searched for information in the following guides.
- [x] Search the issues already opened: https://github.com/GoogleCloudPlatform/google-cloud-node/issues
- [x] Search StackOverflow: http://stackoverflow.com/questions/tagged/google-cloud-platform+node.js
- [x] Check our Troubleshooting guide: https://github.com/googleapis/google-cloud-node/blob/main/docs/troubleshooting.md
- [x] Check our FAQ: https://github.com/googleapis/google-cloud-node/blob/main/docs/faq.md
- [x] Check our libraries HOW-TO: https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md
- [x] Check out our authentication guide: https://github.com/googleapis/google-auth-library-nodejs
- [x] Check out handwritten samples for many of our APIs: https://github.com/GoogleCloudPlatform/nodejs-docs-samples
- [x] Check the API's issue tracker: https://cloud.google.com/support/docs/issue-trackers
### Library Name
@google-cloud/storage
### A screenshot that you have tested with "Try this API".
N/A
### Link to the code that reproduces this issue. A link to a **public** Github Repository or gist with a minimal reproduction.
https://gist.github.com/tcrognon/3102b05ce3e0aa902e22decb5be0a788
### A step-by-step description of how to reproduce the issue, based on the linked reproduction.
1. Replace SOME_BUCKET with your GCP test bucket.
2. Replace SOME_FILE_PATH with a file to upload to SOME_BUCKET.
3. Run the code and somehow cause one of the PUT requests to fail, but don't just disable internet because the DELETE request (to abort the XML multipart upload) needs to succeed.
### A clear and concise description of what the bug is, and what you expected to happen.
`uploadFileInChunks` performs an XML multipart upload. This is:
1) A POST to create the multipart upload.
2) A series of PUTs to upload the parts.
3) A POST to complete the multipart upload.
4) A DELETE to abort the multipart upload if 2) or 3) fails.
If 4) happens and is successful, `uploadFileInChunks` returns `undefined`. I expected it would throw. All other error throw.
Here is the offending `return;`: https://github.com/googleapis/google-cloud-node/blob/b5b5afb9e053744ff7c3f6334aa2a60f86466c84/handwritten/storage/src/transfer-manager.ts#L955
### A clear and concise description WHY you expect this behavior, i.e., was it a recent change, there is documentation that points to this behavior, etc. **
Returning `undefined` in this single case is very unexpected and the documentation heavily implies that if `uploadFileInChunks` doesn't throw, then the file was written successfully:
- https://docs.cloud.google.com/nodejs/docs/reference/storage/latest/storage/transfermanager#_google_cloud_storage_TransferManager_uploadFileInChunks_member_1_
- https://docs.cloud.google.com/storage/docs/multipart-uploads#storage-upload-object-chunks-nodejs
If auth problem, throws. If invalid bucket or file, throws. If initial POST fails, throws. If subsequent PUTs/POST fails and the abort fails, throws. But if the abort succeeds, returns undefined.
I experienced silent data loss due to this. Code continued after aborted uploads thinking they were successfully written.
Contributor guide
Research direction
Start in handwritten/storage/src/transfer-manager.ts at the linked return in uploadFileInChunks, then review the reproduction gist to understand the failed part upload and successful abort. Confirm that an upload failure still propagates an error after the multipart upload is aborted, rather than returning undefined.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- google-cloud, node.js, typescript
- Domain
- backend, cloud
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 84/100