Added a mutex around API methods for concurrent calls #476

natdm · 2017-10-14T16:02:17Z

Fixes the below trace:

==================
WARNING: DATA RACE
Read at 0x000001f96a20 by goroutine 83:
  github.com/natdm/bideasy/vendor/github.com/stripe/stripe-go/order.List()
      /Users/nathanhyland/gocode/src/github.com/natdm/bideasy/vendor/github.com/stripe/stripe-go/stripe.go:172 +0xdf
  github.com/natdm/bideasy/billing.(*Billing).AllOrders()
      /Users/nathanhyland/gocode/src/github.com/natdm/bideasy/billing/billing.go:217 +0xe7
  github.com/natdm/bideasy/controllers.(*Admin).ListOrders.func1()
      /Users/nathanhyland/gocode/src/github.com/natdm/bideasy/controllers/admin.go:1750 +0x139

Previous write at 0x000001f96a20 by goroutine 85:
  github.com/natdm/bideasy/vendor/github.com/stripe/stripe-go/balance.List()
      /Users/nathanhyland/gocode/src/github.com/natdm/bideasy/vendor/github.com/stripe/stripe-go/stripe.go:173 +0x194
  github.com/natdm/bideasy/billing.(*Billing).AllBalances()
      /Users/nathanhyland/gocode/src/github.com/natdm/bideasy/billing/billing.go:245 +0xde
  github.com/natdm/bideasy/controllers.(*Admin).ListOrders.func3()
      /Users/nathanhyland/gocode/src/github.com/natdm/bideasy/controllers/admin.go:1764 +0x136

How to reproduce:

Orders has a Charge object, but it's not filled out. So to get that, I get all the charges. Charges have a Transaction/Balance entry, but also not populated. My code looks like this, with each call to stripe going to a List type of call (ListTransactions, etc):

        type stripeOrderResTyp struct {
		orders map[string]stripe.Order
		err    error
	}
	type stripeBalanceResTyp struct {
		transactions map[string]stripe.Transaction
		err          error
	}
	type stripeChargeResTyp struct {
		charges map[string]stripe.Charge
		err     error
	}

	stripeOrderResCh := make(chan stripeOrderResTyp)
	stripeChargeResCh := make(chan stripeChargeResTyp)
	stripeBalanceResCh := make(chan stripeBalanceResTyp)

	go func() {
		orders, err := a.billing.AllOrders(*admin.StripeCustomerID)
		stripeOrderResCh <- stripeOrderResTyp{orders, err}
	}()

	go func() {
		charges, err := a.billing.AllCharges(*admin.StripeCustomerID)
		stripeChargeResCh <- stripeChargeResTyp{charges, err}
	}()

	go func() {
		balances, err := a.billing.AllBalances(*admin.StripeCustomerID)
		stripeBalanceResCh <- stripeBalanceResTyp{balances, err}
	}()

        ....
        
        // wait for all stripe calls to cease
	stripeOrdersRes := <-stripeOrderResCh
	stripeChargeRes := <-stripeChargeResCh
	stripeBalanceRes := <-stripeBalanceResCh

Looks like the List() call on some of these all touch this bit of code which isn't concurrent safe.

~~I did not include a test with the first commit~~ Turns out I was wrong. Tests aren't being ran with the --race flag, so going to enable that, and make a test for this.

This fixes all of my calls that concurrently call a List() type method.

natdm · 2017-10-14T19:07:09Z

Moved the mutex to the Configuration struct. It threw one race after another when I was trying to be too specific. Once I just added it to the overall call, everything worked out fine and was much cleaner.

Also added --race to the make test and make bench calls. I would add it to make build if you guys want me to.

There's other race possibilities in here as well, like if you run multiple SetBackend calls. To someone doing something like that, I would argue, "you're using it wrong."

natdm · 2017-10-14T19:10:17Z

error_test.go

-		APIBackend,
-		ts.URL,
-		&http.Client{},
+		Type: APIBackend, URL: ts.URL, HTTPClient: &http.Client{},
 	})

 	err := GetBackend(APIBackend).Call("GET", "/v1/account", "sk_test_badKey", nil, nil, nil)


Not something to do in this PR, but probably switching these "GET", and other http method calls to the official http.MethodGet types and all that might be less error prone, -- not that this is currently the source of any issues.

natdm · 2017-10-14T19:36:00Z

I learned something new.

BackendConfiguration needs a Mutex reference to be concurrent safe, but I gave it the value. If I give it a pointer like it asks, it could throw an NPE. I could name it and fix it everywhere, assigning names, but the fact that BackendConfiguration is exported and we're giving it a pointer value to a mutex is both ugly/unfriendly to expose and a breaking change.

Let me know what your thoughts are.

Edit: I changed the method receivers to all be consistent for BackendConfiguration, which removed the need for an addressed mutex. Much cleaner, I think.

natdm · 2017-10-16T18:06:43Z

Sorry about all the comments/commits.

I have no idea how this passed locally on the last few commits. Thank Travis for catching a few hiccups.

brandur-stripe · 2017-10-16T20:21:13Z

stripe.go

@@ -78,6 +79,7 @@ type BackendConfiguration struct {
 	Type       SupportedBackend
 	URL        string
 	HTTPClient *http.Client
+	sync.Mutex


Let's give this a name instead of embedding the struct. mu is usually a good candidate and used elsewhere in the code here.

Will do.

Also, some of the code that looks like I formatted it is just artifacts of my first (naiive) iteration. I can change those back if you wish -- apologies for the "larger than it needs to be" PR.

Maybe hold off just a sec — I'm a little tied up right now, but am in progress of writing a slightly more comprehensive comment on here.

:-X Pushed before I saw this.

brandur-stripe · 2017-10-16T22:48:47Z

Hi @natdm, thanks for taking a crack at this!

This is looking pretty good and I think we should get it merged. The single biggest thing I noticed is that we should move the invocations of our mutex closer to where it's actually needed. You've added a mutex on BackendConfiguration, but if I'm reading this right, the race is in GetBackend which is local to stripe.go itself rather than any particular BackendConfiguration. We should probably move the mutex up into a package-level, and move its use to where it's needed.

This would be my proposal:

diff --git a/stripe.go b/stripe.go
index fd25675..e088978 100644
--- a/stripe.go
+++ b/stripe.go
@@ -14,6 +14,7 @@ import (
 	"os/exec"
 	"runtime"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/stripe/stripe-go/form"
@@ -101,6 +102,7 @@ const (
 // Backends are the currently supported endpoints.
 type Backends struct {
 	API, Uploads Backend
+	mu           sync.RWMutex
 }
 
 // stripeClientUserAgent contains information about the current runtime which
@@ -166,22 +168,39 @@ func NewBackends(httpClient *http.Client) *Backends {
 
 // GetBackend returns the currently used backend in the binding.
 func GetBackend(backend SupportedBackend) Backend {
-	var ret Backend
 	switch backend {
 	case APIBackend:
-		if backends.API == nil {
-			backends.API = BackendConfiguration{backend, apiURL, httpClient}
+		backends.mu.RLock()
+		ret := backends.API
+		backends.mu.RUnlock()
+
+		if ret != nil {
+			return ret
 		}
 
-		ret = backends.API
+		backends.mu.Lock()
+		defer backends.mu.Unlock()
+
+		backends.API = BackendConfiguration{backend, apiURL, httpClient}
+		return backends.API
+
 	case UploadsBackend:
-		if backends.Uploads == nil {
-			backends.Uploads = BackendConfiguration{backend, uploadsURL, httpClient}
+		backends.mu.RLock()
+		ret := backends.Uploads
+		backends.mu.RUnlock()
+
+		if ret != nil {
+			return ret
 		}
-		ret = backends.Uploads
+
+		backends.mu.Lock()
+		defer backends.mu.Unlock()
+
+		backends.Uploads = BackendConfiguration{backend, uploadsURL, httpClient}
+		return backends.Uploads
 	}
 
-	return ret
+	return nil
 }
 
 // SetBackend sets the backend used in the binding.

(I also moved over to an RWMutex so that subsequent read operations after the value is set don't need to block on a lock and are allowed to make concurrent access.)

Thoughts?

A few other notes:

Usually for a patch like this it's best to minimize unrelated changes so that you maximize the signal for the important parts. For example, moving to named fields in struct initialization is probably a good idea, but that can just easily be landed in an unrelated "clean up" pull request that's easy to review.
Before finalizing this, can you rebase your commits so that all changes are squashed into one relative to master? Thanks!

Here's a couple responses in no particular order:

Also added --race to the make test and make bench calls. I would add it to make build if you guys want me to.

Thanks for adding that! It makes sense to add this in the test suite I think, but as noted in its release article, a race-enabled binary uses ~10x CPU and memory, so let's leave it out of the standard build paths.

Edit: I changed the method receivers to all be consistent for BackendConfiguration, which removed the need for an addressed mutex. Much cleaner, I think.

Nice! Yeah, I can't believe we mixed pointer and non-pointer receivers there. They should all be the same.

brandur-stripe · 2017-10-16T22:50:25Z

Makefile

@@ -10,7 +10,7 @@ check-gofmt:
 	scripts/check_gofmt.sh

 test:
-	go test ./...
+	go test --race ./...


Go tends to prefer the BSD-style flags (i.e. one hyphen instead of two, and no short names) for its executables, so let's use -race instead of this as described here. I'm actually fairly surprised that --race even works!

natdm · 2017-10-16T23:40:40Z

Good call on all of that.

I definitely didn't mean to add a larger diff with the named fields -- I should have cleaned that up when I realized it was not needed.

I'll make the requested changes. Thanks for the comments -- just like last time, much cleaner.

brandur-stripe · 2017-10-17T02:10:37Z

stripe_test.go

@@ -22,6 +23,25 @@ func TestCheckinUseBearerAuth(t *testing.T) {
 	assert.Equal(t, "Bearer "+key, req.Header.Get("Authorization"))
 }

+// TestMultipleAPICalls will fail the test run if a race condition is thrown while running multople NewRequest calls.


Very minor, but "multiple" is misspelled here.

brandur-stripe · 2017-10-17T02:11:42Z

I definitely didn't mean to add a larger diff with the named fields -- I should have cleaned that up when I realized it was not needed.

I'll make the requested changes. Thanks for the comments -- just like last time, much cleaner.

No worries at all!

I left one more nit above, but otherwise this LGTM. Thanks!

natdm · 2017-10-17T17:41:11Z

Fixed.

brandur-stripe · 2017-10-17T18:49:05Z

Thanks again!

brandur-stripe · 2017-10-17T18:50:20Z

Released as 28.3.1.

natdm commented Oct 14, 2017

View reviewed changes

natdm changed the title ~~Added a mutex around backend varible for concurrent reads~~ Added a mutex around API methods for concurrent calls Oct 16, 2017

brandur-stripe reviewed Oct 16, 2017

View reviewed changes

natdm force-pushed the backend-mutex branch from d3dcda5 to 2fc83ed Compare October 17, 2017 00:37

brandur-stripe reviewed Oct 17, 2017

View reviewed changes

Added a mutex around backend varible for concurrent reads

e7d2ea7

natdm force-pushed the backend-mutex branch from c5093aa to e7d2ea7 Compare October 17, 2017 02:34

brandur-stripe merged commit 59a24d6 into stripe:master Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a mutex around API methods for concurrent calls #476

Added a mutex around API methods for concurrent calls #476

natdm commented Oct 14, 2017 •

edited

Loading

natdm commented Oct 14, 2017

natdm Oct 14, 2017

natdm commented Oct 14, 2017 •

edited

Loading

natdm commented Oct 16, 2017

brandur-stripe Oct 16, 2017

natdm Oct 16, 2017

brandur-stripe Oct 16, 2017 •

edited

Loading

natdm Oct 16, 2017

brandur-stripe commented Oct 16, 2017 •

edited

Loading

brandur-stripe Oct 16, 2017

natdm commented Oct 16, 2017

brandur-stripe Oct 17, 2017

brandur-stripe commented Oct 17, 2017

natdm commented Oct 17, 2017

brandur-stripe commented Oct 17, 2017

brandur-stripe commented Oct 17, 2017

Added a mutex around API methods for concurrent calls #476

Added a mutex around API methods for concurrent calls #476

Conversation

natdm commented Oct 14, 2017 • edited Loading

Fixes the below trace:

How to reproduce:

natdm commented Oct 14, 2017

natdm Oct 14, 2017

Choose a reason for hiding this comment

natdm commented Oct 14, 2017 • edited Loading

natdm commented Oct 16, 2017

brandur-stripe Oct 16, 2017

Choose a reason for hiding this comment

natdm Oct 16, 2017

Choose a reason for hiding this comment

brandur-stripe Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

natdm Oct 16, 2017

Choose a reason for hiding this comment

brandur-stripe commented Oct 16, 2017 • edited Loading

brandur-stripe Oct 16, 2017

Choose a reason for hiding this comment

natdm commented Oct 16, 2017

brandur-stripe Oct 17, 2017

Choose a reason for hiding this comment

brandur-stripe commented Oct 17, 2017

natdm commented Oct 17, 2017

brandur-stripe commented Oct 17, 2017

brandur-stripe commented Oct 17, 2017

natdm commented Oct 14, 2017 •

edited

Loading

natdm commented Oct 14, 2017 •

edited

Loading

brandur-stripe Oct 16, 2017 •

edited

Loading

brandur-stripe commented Oct 16, 2017 •

edited

Loading