fix(secret-manager): rewrite secret manager to async/await with await of initialize()#6380
Closed
eagleeye wants to merge 1 commit intogoogleapis:mainfrom
Closed
fix(secret-manager): rewrite secret manager to async/await with await of initialize()#6380eagleeye wants to merge 1 commit intogoogleapis:mainfrom
initialize()#6380eagleeye wants to merge 1 commit intogoogleapis:mainfrom
Conversation
9563064 to
09828b6
Compare
initialize()initialize()
617444f to
da51579
Compare
da51579 to
ca4bb7c
Compare
Contributor
Author
|
@sofisl I've fixed some more issues in the code, I've missed few awaits, please trigger test again. |
Contributor
|
/gcbrun |
Contributor
|
@eagleeye Thanks for your contributions. This library isn't where the changes go but I did want to check if tests would pass. I need to confirm with my team what the best move is here but I'll reopen this as an issue in the generator for tracking: googleapis/google-cloud-node-core#543 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi there! I've find a lot if issues with async code, that are causing uncaught exceptions in our production systems, so I've fixed them.
The last change to the secret manager did not improve stability of the code, here I've explained why:
#6188 (review)
And showed it with a small test
List of changes:
async- this helps to track Promises in the code, allows to use await in that functionsthis.initialize()toawait this.initializedawait, that allowed to flatten the codeOriginal error, that happens super rarely, near in 1/1000 times, but it causes our containers restart during start, because it happens out of secret-manager call stack, so we can't retry it.
The error was spotted in secret manager v5.6.0, that is not the latest and I'm aware, that some fixes were added in 6.0.1 with google-gax update, but anyway, current master of secret manager has issues in handling promises, that must be fixed.
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:
The issues with async code are obvious and I believe that they do not require discussions
The code didn't changed the signature of the methods. The methods were async, the tests didn't changed.
Fixes #<issue_number_goes_here> 🦕
(No issue created)