Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test for data race conditions in adapters #1756

Merged
merged 20 commits into from
May 12, 2021
Merged

Test for data race conditions in adapters #1756

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
7074f58
Data race test first draft
Mar 17, 2021
cbcc293
Step out real quick
Mar 18, 2021
a4c6c4a
Rollback CopyWithOption
Mar 18, 2021
dde3442
Implemented bid request deep copy function instead of importing library
Mar 25, 2021
8dc32b2
merge recent modifications to adapters/adapterstest/test_json.go
Mar 25, 2021
e81eceb
resolved conflicts
Mar 25, 2021
015fc30
informative comment
Mar 25, 2021
e22ffd7
Merge branch 'master' into sharedMemoryTest
Mar 26, 2021
4ce81b9
resolved openrtb2 merge conflicts
Mar 26, 2021
eb75ee4
Rolled back to use the github.com/jinzhu/copier library
Apr 8, 2021
87d8461
step out real quick
Apr 16, 2021
4e81fcf
Scott's review and deep copy library switch
Apr 29, 2021
77d9a54
updated with latest master and resolved merge conflicts
Apr 29, 2021
e8d3a6a
Added slice assertions
May 5, 2021
6be78a1
Major corrections. Removed the assertNoDataRace functions entirely
May 6, 2021
f96f9e7
Merge branch 'master' into sharedMemoryTest
May 6, 2021
1023f30
go mod tidy-ed
May 6, 2021
5f81f1d
Scott's latest review. Improving error messages and reducing large co…
May 6, 2021
4138379
Merge branch 'master' into sharedMemoryTest
May 11, 2021
d0e5e70
Modified imp shallow copies comment
May 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 96 additions & 29 deletions adapters/adapterstest/test_json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import (
"regexp"
"testing"

"github.com/mitchellh/copystructure"
"github.com/mxmCherry/openrtb/v15/openrtb2"
"github.com/prebid/prebid-server/adapters"
"github.com/stretchr/testify/assert"
"github.com/yudai/gojsondiff"
"github.com/yudai/gojsondiff/formatter"

Expand Down Expand Up @@ -107,24 +109,10 @@ func runSpec(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidd
} else if isVideoTest {
reqInfo.PbsEntryPoint = "video"
}
actualReqs, errs := bidder.MakeRequests(&spec.BidRequest, &reqInfo)
diffErrorLists(t, fmt.Sprintf("%s: MakeRequests", filename), errs, spec.MakeRequestErrors)
diffHttpRequestLists(t, filename, actualReqs, spec.HttpCalls)

bidResponses := make([]*adapters.BidderResponse, 0)

var bidsErrs = make([]error, 0, len(spec.MakeBidsErrors))
for i := 0; i < len(actualReqs); i++ {
thisBidResponse, theseErrs := bidder.MakeBids(&spec.BidRequest, spec.HttpCalls[i].Request.ToRequestData(t), spec.HttpCalls[i].Response.ToResponseData(t))
bidsErrs = append(bidsErrs, theseErrs...)
bidResponses = append(bidResponses, thisBidResponse)
}

diffErrorLists(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors)
requests := testMakeRequestsImpl(t, filename, spec, bidder, &reqInfo)

for i := 0; i < len(spec.BidResponses); i++ {
diffBidLists(t, filename, bidResponses[i], spec.BidResponses[i].Bids)
}
testMakeBidsImpl(t, filename, spec, bidder, requests)
}

type testSpec struct {
Expand Down Expand Up @@ -194,8 +182,8 @@ type expectedBid struct {
//
// Marshalling the structs and then using a JSON-diff library isn't great either, since

// diffHttpRequests compares the actual http requests to the expected ones.
func diffHttpRequestLists(t *testing.T, filename string, actual []*adapters.RequestData, expected []httpCall) {
// assertMakeRequestsOutput compares the actual http requests to the expected ones.
func assertMakeRequestsOutput(t *testing.T, filename string, actual []*adapters.RequestData, expected []httpCall) {
t.Helper()

if len(expected) != len(actual) {
Expand All @@ -206,7 +194,7 @@ func diffHttpRequestLists(t *testing.T, filename string, actual []*adapters.Requ
}
}

func diffErrorLists(t *testing.T, description string, actual []error, expected []testSpecExpectedError) {
func assertErrorList(t *testing.T, description string, actual []error, expected []testSpecExpectedError) {
t.Helper()

if len(expected) != len(actual) {
Expand All @@ -227,10 +215,10 @@ func diffErrorLists(t *testing.T, description string, actual []error, expected [
}
}

func diffBidLists(t *testing.T, filename string, response *adapters.BidderResponse, expected []expectedBid) {
func assertMakeBidsOutput(t *testing.T, filename string, bidderResponse *adapters.BidderResponse, expected []expectedBid) {
t.Helper()

if (response == nil || len(response.Bids) == 0) != (len(expected) == 0) {
if (bidderResponse == nil || len(bidderResponse.Bids) == 0) != (len(expected) == 0) {
if len(expected) == 0 {
t.Fatalf("%s: expectedBidResponses indicated a nil response, but mockResponses supplied a non-nil response", filename)
}
Expand All @@ -239,17 +227,15 @@ func diffBidLists(t *testing.T, filename string, response *adapters.BidderRespon
}

// Expected nil response - give diffBids something to work with.
if response == nil {
response = new(adapters.BidderResponse)
if bidderResponse == nil {
bidderResponse = new(adapters.BidderResponse)
}

actual := response.Bids

if len(actual) != len(expected) {
t.Fatalf("%s: MakeBids returned wrong bid count. Expected %d, got %d", filename, len(expected), len(actual))
if len(bidderResponse.Bids) != len(expected) {
t.Fatalf("%s: MakeBids returned wrong bid count. Expected %d, got %d", filename, len(expected), len(bidderResponse.Bids))
}
for i := 0; i < len(actual); i++ {
diffBids(t, fmt.Sprintf("%s: typedBid[%d]", filename, i), actual[i], &(expected[i]))
for i := 0; i < len(bidderResponse.Bids); i++ {
diffBids(t, fmt.Sprintf("%s: typedBid[%d]", filename, i), bidderResponse.Bids[i], &(expected[i]))
}
}

Expand Down Expand Up @@ -331,3 +317,84 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte)
}
}
}

// testMakeRequestsImpl asserts the resulting values of the bidder's `MakeRequests()` implementation
// against the expected JSON-defined results and makes sure we do not incurr in data races in the
// process. To verify no data races occur this function creates:
// 1) A shallow copy of the unmarshalled openrtb2.BidRequest that will provide reference values to
// shared memory that we don't want the adapters' implementation of `MakeRequests()` to modify.
// 2) A deep copy that will preserve the original values of all the fields. This copy remains untouched
// by the adapters' processes and serves as reference of what the shared memory values should still
// be after the `MakeRequests()` call.
//
// The original values stored in the deep copy will be compared against the shared memory values and
// discrepancies will point to data race conditions in an adapter's `MakeRequests()` implementation.
//
// Because neither the shallow nor the deep copies are passed to `MakeRequests()` we know static fields
// in openrtb2.BidRequest nor openrtb2.Imp elements will change, therefore, we can reliably compare
// reference values using the `assert.Equal()` method.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't end up liking this huge comment. It probably needs rewording. Suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO the last two paragraphs can be removed. One other suggestion:

  • "makes sure we do not incurr in data races in the process" -> "ensures we do not encounter data races in the process"

func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, reqInfo *adapters.ExtraRequestInfo) []*adapters.RequestData {
t.Helper()

deepBidReqCopy, shallowBidReqCopy, err := getDataRaceTestCopies(&spec.BidRequest)
assert.NoError(t, err, "Could not create deep copy for data race assertions. %s\n", filename)
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

// Run MakeRequests
requests, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo)

// Compare MakeRequests actual output versus expected values found in JSON file
assertErrorList(t, fmt.Sprintf("%s: MakeRequests", filename), errs, spec.MakeRequestErrors)
assertMakeRequestsOutput(t, filename, requests, spec.HttpCalls)

// Assert no data races occur using original bidRequest copies of references and values
assert.Equal(t, deepBidReqCopy, shallowBidReqCopy, "Data race found. Test: %s \n", filename)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After many local tests, local adapter modifications to purposefully create race conditions, and trying out many deep copy libraries available out there: we finally found the mix to simply assert.Equal() instead of writing an assert function of our own. The only downside of not using our previously implemented assertNoDataRaces(...) function is the less descriptive "Data race found. Test: %s \n" message compared to something like: "Data race in bidRequest.Imp[0].Native field. File: %s \n". The latter would point to the exact shared memory field being modified.

I still think it's worth it though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine. Testisy will highlight the differences between the expected and actual.


return requests
}

// getDataRaceTestCopies returns a deep copy and a shallow copy of the original bidRequest that will get
// compared to verify no data races occur.
func getDataRaceTestCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest, error) {
cpy, err := copystructure.Copy(original)
if err != nil {
return nil, nil, err
}
deepReqCopy := cpy.(*openrtb2.BidRequest)

shallowReqCopy := *original

// If we call `copy` on a nil []Imp, the shallowReqCopy.Imp slice will initialize to openrtb2.Imp{}
// and not nil, which will lead to false positives.
if original.Imp != nil {
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
shallowReqCopy.Imp = make([]openrtb2.Imp, len(original.Imp))
copy(shallowReqCopy.Imp, original.Imp)
}

return deepReqCopy, &shallowReqCopy, nil
}

// testMakeBidsImpl asserts the results of the bidder MakeBids implementation against the expected JSON-defined results
func testMakeBidsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, makeRequestsOut []*adapters.RequestData) {
t.Helper()

bidResponses := make([]*adapters.BidderResponse, 0)
var bidsErrs = make([]error, 0, len(spec.MakeBidsErrors))

// We should have as many bids as number of adapters.RequestData found in MakeRequests output
for i := 0; i < len(makeRequestsOut); i++ {
// Run MakeBids with JSON refined spec.HttpCalls info that was asserted to match MakeRequests
// output inside testMakeRequestsImpl
thisBidResponse, theseErrs := bidder.MakeBids(&spec.BidRequest, spec.HttpCalls[i].Request.ToRequestData(t), spec.HttpCalls[i].Response.ToResponseData(t))

bidsErrs = append(bidsErrs, theseErrs...)
bidResponses = append(bidResponses, thisBidResponse)
}

// Assert actual errors thrown by MakeBids implementation versus expected JSON-defined spec.MakeBidsErrors
assertErrorList(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors)

// Assert MakeBids implementation BidResponses with expected JSON-defined spec.BidResponses[i].Bids
for i := 0; i < len(spec.BidResponses); i++ {
assertMakeBidsOutput(t, filename, bidResponses[i], spec.BidResponses[i].Bids)
}
}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ require (
github.com/magiconair/properties v1.8.0
github.com/mattn/go-colorable v0.1.2 // indirect
github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
github.com/mitchellh/copystructure v1.1.2
github.com/mitchellh/mapstructure v1.0.0 // indirect
github.com/mxmCherry/openrtb/v15 v15.0.0
github.com/pelletier/go-toml v1.2.0 // indirect
Expand Down
11 changes: 4 additions & 7 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,12 @@ github.com/mattn/go-isatty v0.0.8 h1:HLtExJ+uU2HOZ+wI0Tt5DtUDrx8yhUqDcp7fYERX4CE
github.com/mattn/go-isatty v0.0.8/go.mod h1:Iq45c/XA43vh69/j3iqttzPXn0bhXyGjM0Hdxcsrc5s=
github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0jegS5sx/RkqARlsWZ6pIwiU=
github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0=
github.com/mitchellh/copystructure v1.1.2 h1:Th2TIvG1+6ma3e/0/bopBKohOTY7s4dA8V2q4EUcBJ0=
github.com/mitchellh/copystructure v1.1.2/go.mod h1:EBArHfARyrSWO/+Wyr9zwEkc6XMFB9XyNgFNmRkZZU4=
github.com/mitchellh/mapstructure v1.0.0 h1:vVpGvMXJPqSDh2VYHF7gsfQj8Ncx+Xw5Y1KHeTRY+7I=
github.com/mitchellh/mapstructure v1.0.0/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y=
github.com/mitchellh/reflectwalk v1.0.1 h1:FVzMWA5RllMAKIdUSC8mdWo3XtwoecrH79BY70sEEpE=
github.com/mitchellh/reflectwalk v1.0.1/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw=
github.com/mxmCherry/openrtb/v15 v15.0.0 h1:inLuQ3Bsima9HLB2v6WjbtEFF69SWOT5Dux4QZtYdrw=
github.com/mxmCherry/openrtb/v15 v15.0.0/go.mod h1:TVgncsz6MOzbL7lhun1lNuUBzVBlVDbxf9Fyy1TyhZA=
github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A=
Expand Down Expand Up @@ -123,7 +127,6 @@ github.com/spf13/viper v1.1.0/go.mod h1:A8kyI5cUJhb8N+3pkfONlcEcZbueH6nhAm0Fq7Sr
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.1.1 h1:2vfRuCMp5sSVIDSqO8oNnWJq7mPa6KVP3iPIwFBuy8A=
github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4=
github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
Expand All @@ -146,7 +149,6 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/net v0.0.0-20180906233101-161cd47e91fd h1:nTDtHvHSdCn1m6ITfMRqtOd/9+7a3s8RBNOZ3eYZzJA=
golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
Expand All @@ -157,10 +159,8 @@ golang.org/x/net v0.0.0-20201202161906-c7110b5ffcbb/go.mod h1:sp8m0HH+o8qH0wwXwY
golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e h1:o3PsSEY8E4eXWkXrIP9YJALUkVZqzHJT5DOasTyn8Vs=
golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190222072716-a9d3bda3a223 h1:DH4skfRX4EBpamg7iV4ZlCpblAHI6s6TDM39bFZumv8=
golang.org/x/sys v0.0.0-20190222072716-a9d3bda3a223/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20190904154756-749cb33beabd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
Expand All @@ -170,7 +170,6 @@ golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7w
golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210112080510-489259a85091 h1:DMyOG0U+gKfu8JZzg2UQe9MeaC1X+xQWlAKcRnjxjCw=
golang.org/x/sys v0.0.0-20210112080510-489259a85091/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.3 h1:cokOdA+Jmi5PJGXLlLllQSgYigAEfHXJAERHVMaCc2k=
golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
Expand All @@ -191,11 +190,9 @@ google.golang.org/protobuf v1.23.0 h1:4MY060fB1DLGMB/7MBTLnwQUY6+F09GEiz6SsrNqyz
google.golang.org/protobuf v1.23.0/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2l/sGQquU=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/fsnotify.v1 v1.4.7 h1:xOHLXZwVvI9hhs+cLKq5+I5onOuwQLhQwiu63xxlHs4=
gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys=
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ=
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw=
gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
gopkg.in/yaml.v2 v2.3.0/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
Expand Down