Fix data race surrounding Client.Rate.

[dmitshur]

Previously, concurrent calls to Client.Do would have data races writing Client.Rate. Protect it with a mutex.

Resolves #253.

I've been using/testing it for the last week or two, and I haven't spotted any other issues with concurrency.

Technically, this is a ever-so-slightly breaking API change, since Rate was previously an exported struct field, but now it's a method, so callers will need to change client.Rate to client.Rate(). I don't see any other way around this. Aside from exporting the mutex and keeping Rate as is, but that would require the user to know about the mutex and lock it when reading Rate. That's very unfriendly API.

[dmitshur]

So far, it's just a simple patch for client.Rate without any documentation change, because I wanted to ask you first if you'd want to add something like this?

--- a/github/github.go
+++ b/github/github.go
@@ -50,6 +50,9 @@ const (
 )

 // A Client manages communication with the GitHub API.
+//
+// Clients are safe for concurrent use by multiple goroutines, but a single Client
+// shouldn't be used to make calls on behalf of different users.
 type Client struct {
    // HTTP client used to communicate with the API.
    client *http.Client

Also, due to the nature of this PR, it should be reviewed more carefully than a simple GitHub API coverage addition PR.

[willnorris]

any reason not to use a sync.RWMutex here so that Rate() can just perform a read lock?

[dmitshur]

Yes, I chose sync.Mutex over sync.RWMutex here on purpose, because I estimated that it'd be simpler and faster, and hence more appropriate.

I've seen some arguments and articles demonstrating that the more complex read-write locks are often slower than the simpler mutexes for basic use cases:

• https://blogs.oracle.com/roch/entry/beware_of_the_performance_of
• http://preshing.com/20111124/always-use-a-lightweight-mutex/
• http://askldjd.com/2011/03/06/performance-comparison-on-reader-writer-locks/

I was convinced to at least use sync.Mutex by default, and upgrade to sync.RWMutex after doing profiling and ensuring it's actually faster and better.

If you prefer I change this to sync.RWMutex, I can do that.

[willnorris]

no, this is fine.

Though I would certainly be curious to see benchmarks of the two, not just for this library but in general. I just wonder how much difference it really makes in practice.

[jabley]

👍

Thank you, I was just running https://github.com/jabley/project-status and hitting the race condition. Quick update and sorted.