Fix suspected race condition in prompt handling #105
Merged
Conversation
This file contains 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
The current approach to handling prompts involves calling for a prompt and then registering a signal handler for a response that indicates the prompt had completed. In most cases, if the user were to take action, this would be quite slow. However, in the case of automatic responses to the prompt, the signal response may appear before the signal handler is configured, resulting in blocking forever. This commit moves the call for a prompt after the signal handling has been configured, closing the race window. Signed-off-by: William Martin <williammartin@github.com>
This was referenced Mar 15, 2024
gh tries to use hosts.yml after initial boot and fails, then tries keyring and succeeds
cli/cli#8802
Closed
williammartin
changed the title
|
@williammartin thanks for the PR, the description and the detailed issue, really great work! |
|
👍 |
1 similar comment
|
👍 |
|
Thanks so much for the quick review and merge! Will you create a new release with this or should I pin to the sha in the meantime?
OSS maintainers have to stick together 😉 |
|
I have created a new release! https://github.com/zalando/go-keyring/releases/tag/v0.2.4 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Fixes #104
The current approach to handling prompts involves calling for a prompt and then registering a signal handler for a response that indicates the prompt had completed. In most cases, if the user were to take action, this would be quite slow. However, in the case of automatic responses to the prompt, the signal response may appear before the signal handler is configured, resulting in blocking forever.
This commit moves the call for a prompt after the signal handling has been configured, closing the race window.
Reviewer Notes
I'll admit that I'm not very familiar with all the nuances of dbus or this library so it's entirely possible I've missed something. We've had three reports over on cli/cli#8802 that this has resolved the issue they were facing but I can't say for sure that there's not some unintended side effects.