Suggestion: Make http request signing safe from multiple goroutines

[balboah]

Since working with the http.Client and creating new requests is usually safe from multiple goroutines, I would like to suggest either guarding the things in awsauth that manipulates http.Requests with a mutex or getting rid of the shared states as well.

I'm not entirely sure which parts of it might need changes but the race detector complained on using Sign4 for my requests on multiple routines.

[mholt]

This is a valid concern. I'll look into it soon, and others are welcome to as well. Thanks for bringing it up!

[peterbourgon]

That synchronizes the signing, but clients still must write to the package global Keys manually whenever credentials are updated. If that happens more often than at program start (i.e. in func init) they still need to bring their own synchronization, to prevent data races.

[mholt]

@peterbourgon This is true. I just glanced at the code again, and the only time that I can see that the credentials are updated within the package is when a signing request is made, and fortunately, those are all within the mutex (the checkKeys() function). To update credentials yourself, then, yes, using your own mutex would be a good idea. (I think my logic is okay here, but I may be wrong since I haven't tested that scenario.)

[peterbourgon]

To update credentials yourself, then, yes, using your own mutex would be a good idea.

Well, it's not just a good idea—it's a data race if you don't. Actually, even if you claimed that local clientMutex, another goroutine could still call a Sign method, claim the package signMutex, which could cause a mutation and trigger another data race :(

Problems, as I see them:

1. checkKeys() implies read-only access; that it mutates Keys is very confusing!
2. checkKeys() relies on the caller claiming the signMutex—very fragile.
3. checkKeys() pulling information from os.Getenv is spooky action at a distance :(

I'm afraid the current architecture is inherently unsafe. The Keys credentials should be wrapped (i.e. not available as a package global), explicitly initialized (i.e. no implicit os.Getenv), and all access synchronized through a clear API boundary provided by the package.

[mholt]

Is changing the credentials something you do frequently?

[peterbourgon]

Of course. Imagine a program that uses multiple credentials to make distinct requests on different periodic intervals.

[mholt]

Alright, we'll use issue #14 to resolve this; thanks!