Add timeouts to keyring operations #7580

samcoe · 2023-06-14T02:05:20Z

This PR adds some timeouts to keyring operations that have been reported as hanging in some WSL and Windows machines. The underlying issue stems from the go-keyring package that we use so until the bugs get fixed there we should properly handle the cases where it hangs.

Fixes #7208

samcoe · 2023-06-14T02:05:51Z

internal/keyring/keyring.go

+
+// Set secret in keyring for user.
+func Set(service, user, secret string) error {
+	duration := time.Second


Open to other times, 1 second seemed reasonable though.

williammartin · 2023-06-19T17:16:23Z

internal/keyring/keyring.go

+}
+
+// Get secret from keyring given service and user name.
+func Get(service, user string) (string, error) {


I'm trying to figure out if this function is racy or not. See this playground: https://go.dev/play/p/hCpEywlYjiR which you can run a few times to see different results).

I think it is racy, like the example in the playground, but I can't quite convince myself that time.Sleep doesn't have some impact on goroutine scheduling. Either way, unless you're sure this code isn't racy then I think we should change it. At the heart of the issue is that select can choose any channel branch that succeeds, and putting a value on one channel can race with closing the other, e.g. errCh <- err racing with close(resCh).

There are a few possible (I think) fixes for this:

Using unbuffered channels will block the write until someone reads, preventing the close until a branch has already been picked

Move the closes until after a decision has been made

Use a result struct type result struct { secret string, error string } or anonymous struct c := make(chan struct { string; error }) and only have a single channel

I would favour 3, as any use of multiple channels has the propensity to introduce these kinds of hard to reason about issues and there's probably a reason I've seen it used so many times.

On the other hand, if you know a reason this isn't racy then please hit me with that knowledge!

Great catch here, I do think there is a race condition! I had not thought about how select would work with defer close. I was thinking about using option 3 originally, seems like the best solution, I will make the change.

williammartin · 2023-06-20T12:56:12Z

internal/keyring/keyring.go

+}
+
+// Set secret in keyring for user.
+func Set(service, user, secret string) error {


Another option here that might be a minor improvement would be to remove the context in favour of a time.After i.e.

func Set(service, user, secret string) error { ch := make(chan error, 1) go func() { defer close(ch) ch <- keyring.Set(service, user, secret) }() select { case err := <- ch: return err case <- time.After(time.Second) return &TimeoutError{"timeout while trying to set secret in keyring"} }

The reason I say minor improvement is that I think it's a touch more explicit. When I see a context I generally expect it to have come from somewhere and to be going somewhere. It also doesn't come with a footgun of not calling cancel.

TIL time.After. Great suggestion. Implemented.

williammartin

Thanks for making the changes. I think maybe a little longer delay would be good in case there are legitimately slow systems but it's a total finger in the air.

I left one more totally non-blocking comment which you are free to ignore.

It's a shame that all the go-keyring stuff is managed by init and with private variables and types, so we can't inject any timeout behaviour into the mock.

Add timeouts to keyring operations

8a43e08

samcoe self-assigned this Jun 14, 2023

samcoe requested a review from a team as a code owner June 14, 2023 02:05

samcoe requested review from vilmibm and removed request for a team June 14, 2023 02:05

samcoe commented Jun 14, 2023

View reviewed changes

williammartin reviewed Jun 19, 2023

View reviewed changes

Fix race condition

bf3bcde

samcoe requested a review from williammartin June 20, 2023 00:45

williammartin reviewed Jun 20, 2023

View reviewed changes

williammartin approved these changes Jun 20, 2023

View reviewed changes

Simplify implementation

50fd4de

samcoe enabled auto-merge (squash) June 20, 2023 23:32

samcoe merged commit bf7db84 into trunk Jun 20, 2023

samcoe deleted the keyring-timeout branch June 20, 2023 23:45

This comment was marked as spam.

Sign in to view

renovate bot mentioned this pull request Feb 16, 2024

chore(deps): update dependency github-cli to v2.72.0 sourcegraph/eslint-config#295

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeouts to keyring operations #7580

Add timeouts to keyring operations #7580

This comment was marked as spam.

Add timeouts to keyring operations #7580

Add timeouts to keyring operations #7580

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.