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.
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
fix: force setMetadata calls to use promise version, invoke callbacks manually #2000
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a description to this PR to explain why this change is needed? :)
src/bucket.ts
Outdated
this.setMetadata({lifecycle: {rule: newLifecycleRules}}, callback); | ||
this.storage.retryOptions.autoRetry = this.instanceRetryValue; | ||
this.setMetadata({lifecycle: {rule: newLifecycleRules}}) | ||
.then(resp => callback!(null, resp)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will callbacks always be provided? (guessing yes, but just wanted to double check this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some situations there is no guarantee, in those places I have added a check and utilize util.noop
as the callback.
@@ -3654,22 +3654,6 @@ describe('File', () => { | |||
}); | |||
}); | |||
|
|||
it('should execute callback with error & API response', done => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference, spoke with @ddelgrosso1 and determined that this case does not exist and that it would be safe to remove this test.
Marking do not merge until I fix this up and run a bunch of tests to make sure this doesn't break current behavior. |
Been running some tests locally and from what I can tell things look the same as they were. @danielbankhead would you mind giving this PR a look as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; maybe in a future refactor we can isolate the method-level retry count from the instance's retry count (perhaps via an optional param or property)
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕
Rationale for this change:
setMetadata
is a conditionally idempotent operation. Currently if no precondition is provided we flip a flag to turn off retries. However, sincesetMetadata
is promisified we are seeing behavior where the flag is getting flipped back before the operation completes. This is causing incorrect behavior. I am changing the internal calls to wait for the promise to finish before flipping the flag back.