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.

Sign up for GitHub

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

Jump to bottom

bucket.upload doesn't retry failed resumable requests #1133

Closed
rhodgkins opened this issue Mar 27, 2020 · 1 comment · Fixed by #1135
Closed

bucket.upload doesn't retry failed resumable requests #1133

rhodgkins opened this issue Mar 27, 2020 · 1 comment · Fixed by #1135
Assignees
@stephenplusplus
Labels
api: storage Issues related to the googleapis/nodejs-storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@rhodgkins
Copy link
Contributor

Environment details

  • OS: any
  • Node.js version: any
  • npm version: any
  • @google-cloud/storage version: 4.6.0

Steps to reproduce

  1. Use bucket.upload with a file size over the RESUMABLE_THRESHOLD to upload
  2. If /home/.config is not writable the upload fails

Do the same steps but with file.createWriteStream and a file stream and the upload works.

From looking at the code, bucket.upload sets options.resumable and that means the retry check in file.createWriteStream doesn't work.

Note I discovered this issue by trying a resumable upload on Cloud Run which doesn't allow writing to /home/.config which doesn't seem right!
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Mar 27, 2020
@bcoe bcoe added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Mar 27, 2020
@rhodgkins
Copy link
Contributor Author

rhodgkins commented Mar 27, 2020 •
edited

Looking at this again - a simple fix would be only to set options.resumable to false in bucket.upload if the file is smaller than the threshold. Then if the file is over the threshold, file.createWriteStream will automatically default to trying a resumable upload first.

@bcoe is this a acceptable solution - happy to do a PR if it is?

rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Mar 28, 2020
@rhodgkins
Failing test for googleapis#1133
e049649
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Mar 28, 2020
@rhodgkins
Only disable resumable uploads for bucket.upload when inspecting the …
2449961 
…file size (fixes googleapis#1133)
@rhodgkins rhodgkins mentioned this issue Mar 28, 2020
Merged
4 tasks
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Mar 28, 2020
@rhodgkins
test(bucket): failing test for googleapis#1133
3fe40ae
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Mar 28, 2020
@rhodgkins
fix(bucket): only disable resumable uploads for bucket.upload when in…
2ba1063 
…specting the file size (fixes googleapis#1133)
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Apr 23, 2020
@rhodgkins
Merge branch 'master' into fix-googleapis#1133
9480a29
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Apr 23, 2020
@rhodgkins
test(bucket): failing test for googleapis#1133
5caaa0a
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Apr 23, 2020
@rhodgkins
fix(bucket): only disable resumable uploads for bucket.upload when in…
3948e91 
…specting the file size (fixes googleapis#1133)
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Apr 29, 2020
@rhodgkins
test(bucket): failing test for googleapis#1133
b2aa938
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Apr 29, 2020
@rhodgkins
fix(bucket): only disable resumable uploads for bucket.upload when in…
3399efc 
…specting the file size (fixes googleapis#1133)
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Apr 30, 2020
@rhodgkins
test(bucket): failing test for googleapis#1133
c4a2aed
rhodgkins added a commit to bookcreator/nodejs-storage that referenced this issue Apr 30, 2020
@rhodgkins
fix(bucket): only disable resumable uploads for bucket.upload when in…
71b2321 
…specting the file size (fixes googleapis#1133)
stephenplusplus pushed a commit that referenced this issue May 1, 2020
@rhodgkins
fix(bucket): Only disable resumable uploads for bucket.upload (fixes #…
2c20148 
…1133) (#1135)

* test(bucket): failing test for #1133

* fix(bucket): only disable resumable uploads for bucket.upload when inspecting the file size (fixes #1133)

* text(bucket): Applied patch from @stephenplusplus - see #1135 (comment)
gcf-owl-bot bot added a commit that referenced this issue Jun 24, 2021
@gcf-owl-bot
chore: Remove commit body and PR# from post-processor-changes-txt (#1137
1a9b098 
)

Fixes #1134 🦕

Removes the commit body and relative PR# from the commit message.

For example, for this commit: googleapis/synthtool@9763f20

`post-processor-changes.txt` would contain

```
build: enable npm for php/python builds

Source-Link: googleapis/synthtool@9763f20
```

instead of

```
build: enable npm for php/python builds (#1133)

* build: enable npm for php/python builds

* update comment
Source-Link: googleapis/synthtool@9763f20
```
Source-Link: googleapis/synthtool@e934b93
Post-Processor: gcr.io/repo-automation-bots/owlbot-nodejs:latest@sha256:805e2e389eafefa5ed484c30b83a7a875e6b1c7ee125d812e8b01ecc531c3fac
gcf-merge-on-green bot pushed a commit that referenced this issue Jun 24, 2021
@gcf-owl-bot
build(node): don't throw on deprecation in unit tests (#1494)
dfd1ec7 
Fixes #1134 🦕

Removes the commit body and relative PR# from the commit message.

For example, for this commit: googleapis/synthtool@9763f20

`post-processor-changes.txt` would contain

```
build: enable npm for php/python builds

Source-Link: googleapis/synthtool@9763f20
```

instead of

```
build: enable npm for php/python builds (#1133)

* build: enable npm for php/python builds

* update comment
Source-Link: googleapis/synthtool@9763f20
```
Source-Link: googleapis/synthtool@e934b93
Post-Processor: gcr.io/repo-automation-bots/owlbot-nodejs:latest@sha256:805e2e389eafefa5ed484c30b83a7a875e6b1c7ee125d812e8b01ecc531c3fac
@rhodgkins rhodgkins mentioned this issue Jul 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stephenplusplus stephenplusplus

Labels
api: storage Issues related to the googleapis/nodejs-storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
3 participants
@bcoe @stephenplusplus @rhodgkins