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

Jump to bottom

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 1 commit
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
117 changes: 106 additions & 11 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/jinzhu/copier"
"github.com/mxmCherry/openrtb"
"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,23 +109,67 @@ 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)
actualReqs := testMakeRequestsImpl(t, filename, spec, bidder, &reqInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

The actualReqs name seems a little odd to me. Can we just call this reqs or requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


testMakeBidsImpl(t, filename, spec, bidder, actualReqs)
}

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

// Save original bidRequest values to assert no data races occur inside MakeRequests latter
deepBidReqCopy := openrtb.BidRequest{}
copier.Copy(&deepBidReqCopy, &spec.BidRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

The library has a deep copy option which is not enabled by default. Does it make sense to use that here?

Copy link
Contributor Author

@guscarreon guscarreon Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

After running some tests, the difference I found in results between copier.CopyWithOption, and copier.Copy is that copier.CopyWithOption(&deepBidReqCopy, &spec.BidRequest, copier.Option{DeepCopy: true}) initializes slices even if they were nil in the original object. For instance types Cat []string, SectionCat []string, PageCat []string part of BidRequest.Site get initialized as empty non-nil slices:

   Diff:
   --- Expected
   +++ Actual
   @@ -4,8 +4,5 @@
     Domain: (string) "",
   - Cat: ([]string) {
   - },
   - SectionCat: ([]string) {
   - },
   - PageCat: ([]string) {
   - },
   + Cat: ([]string) <nil>,
   + SectionCat: ([]string) <nil>,
   + PageCat: ([]string) <nil>,
     Page: (string) "",

It also initializes nil Ext objects to empty json.RawMessage

                Diff:
                --- Expected
                +++ Actual
                @@ -31,4 +31,3 @@
                  MACMD5: (string) "",
                - Ext: (json.RawMessage) {
                - }
                + Ext: (json.RawMessage) <nil>
                 })
Test:           TestJsonSamples
Messages:       Data race in BidRequest.Device field

" github.com/getlantern/deepcopy" on the other hand, does not initializes nil slices, which is the functionality that we are looking for.

shallowBidReqCopy := spec.BidRequest

// Save original []Imp elements to assert no data races occur inside MakeRequests latter
deepImpCopies := make([]openrtb.Imp, len(spec.BidRequest.Imp))
shallowImpCopies := make([]openrtb.Imp, len(spec.BidRequest.Imp))
for i := 0; i < len(spec.BidRequest.Imp); i++ {
deepImpCopy := openrtb.Imp{}
copier.Copy(&deepImpCopy, &spec.BidRequest.Imp[i])
deepImpCopies = append(deepImpCopies, deepImpCopy)

shallowImpCopy := spec.BidRequest.Imp[i]
shallowImpCopies = append(shallowImpCopies, shallowImpCopy)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you don't need the imp copying logic using the deep copy option?

Copy link
Contributor Author

@guscarreon guscarreon Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You are right. Simplified


// Run MakeRequests
actualReqs, 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, actualReqs, spec.HttpCalls)

// Assert no data races occur using original bidRequest copies of references and values
assertNoDataRace(t, &deepBidReqCopy, &shallowBidReqCopy, deepImpCopies, shallowImpCopies)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to pass the imps separately from the request? Can the assertNoDataRace access the imps with .Imps?


return actualReqs
}

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))
for i := 0; i < len(actualReqs); i++ {

// 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)
}

diffErrorLists(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors)
// 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++ {
diffBidLists(t, filename, bidResponses[i].Bids, spec.BidResponses[i].Bids)
assertMakeBidsOutput(t, filename, bidResponses[i].Bids, spec.BidResponses[i].Bids)
}
}

Expand Down Expand Up @@ -194,8 +240,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 +252,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,7 +273,7 @@ func diffErrorLists(t *testing.T, description string, actual []error, expected [
}
}

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

if len(actual) != len(expected) {
Expand Down Expand Up @@ -316,3 +362,52 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte)
}
}
}

// assertNoDataRace compares the contents of the reference fields found in the original openrtb.BidRequest to their
// original values to make sure they were not modified and we are not incurring indata races. In order to assert
// no data races occur in the []Imp array, we call assertNoImpsDataRace()
func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb.BidRequest, bidRequestAfter *openrtb.BidRequest, impsBefore []openrtb.Imp, impsAfter []openrtb.Imp) {
t.Helper()

// Assert reference fields were not modified by bidder adapter MakeRequests implementation
assert.Equal(t, bidRequestBefore.Site, bidRequestAfter.Site, "Data race in BidRequest.Site field")
assert.Equal(t, bidRequestBefore.App, bidRequestAfter.App, "Data race in BidRequest.App field")
assert.Equal(t, bidRequestBefore.Device, bidRequestAfter.Device, "Data race in BidRequest.Device field")
assert.Equal(t, bidRequestBefore.User, bidRequestAfter.User, "Data race in BidRequest.User field")
assert.Equal(t, bidRequestBefore.Source, bidRequestAfter.Source, "Data race in BidRequest.Source field")
assert.Equal(t, bidRequestBefore.Regs, bidRequestAfter.Regs, "Data race in BidRequest.Regs field")

// Assert slice fields were not modified by bidder adapter MakeRequests implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add in this comment that the Imp[] slice is purposefully excluded.

assert.ElementsMatch(t, bidRequestBefore.WSeat, bidRequestAfter.WSeat, "Data race in BidRequest.[]WSeat array")
assert.ElementsMatch(t, bidRequestBefore.BSeat, bidRequestAfter.BSeat, "Data race in BidRequest.[]BSeat array")
assert.ElementsMatch(t, bidRequestBefore.Cur, bidRequestAfter.Cur, "Data race in BidRequest.[]Cur array")
assert.ElementsMatch(t, bidRequestBefore.WLang, bidRequestAfter.WLang, "Data race in BidRequest.[]WLang array")
assert.ElementsMatch(t, bidRequestBefore.BCat, bidRequestAfter.BCat, "Data race in BidRequest.[]BCat array")
assert.ElementsMatch(t, bidRequestBefore.BAdv, bidRequestAfter.BAdv, "Data race in BidRequest.[]BAdv array")
assert.ElementsMatch(t, bidRequestBefore.BApp, bidRequestAfter.BApp, "Data race in BidRequest.[]BApp array")
assert.ElementsMatch(t, bidRequestBefore.Ext, bidRequestAfter.Ext, "Data race in BidRequest.[]Ext array")
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 this looks right. I don't recall exactly how Go handles slice fields during a shallow copy. Why wouldn't assert.Equal work correctly here?

Copy link
Contributor Author

@guscarreon guscarreon Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

assert.Equal(t,deepCopy,shallowCopy) will fail because the github.com/getlantern/deepcopy seems to use unmarshal and marshal and the comparison on the Ext fields will fail like:

   Diff:
   --- Expected
   +++ Actual
   @@ -51,7 +51,13 @@
       Exp: (int64) 0,
   -   Ext: (json.RawMessage) (len=64) {
   -    00000000  7b 22 62 69 64 64 65 72  22 3a 7b 22 62 69 64 66  |{"bidder":{"bidf|
   -    00000010  6c 6f 6f 72 22 3a 33 2e  30 31 2c 22 61 70 70 49  |loor":3.01,"appI|
   -    00000020  64 73 22 3a 7b 22 62 61  6e 6e 65 72 22 3a 22 74  |ds":{"banner":"t|
   -    00000030  68 65 57 72 6f 6e 67 41  70 70 49 64 22 7d 7d 7d  |heWrongAppId"}}}|
   +   Ext: (json.RawMessage) (len=153) {
   +    00000000  7b 0a 20 20 20 20 20 20  20 20 20 20 22 62 69 64  |{.          "bid|
   +    00000010  64 65 72 22 3a 20 7b 0a  20 20 20 20 20 20 20 20  |der": {.        |
   +    00000020  20 20 20 20 22 62 69 64  66 6c 6f 6f 72 22 3a 20  |    "bidfloor": |
   +    00000030  33 2e 30 31 2c 0a 20 20  20 20 20 20 20 20 20 20  |3.01,.          |
   +    00000040  20 20 22 61 70 70 49 64  73 22 3a 20 7b 0a 20 20  |  "appIds": {.  |
   +    00000050  20 20 20 20 20 20 20 20  20 20 20 20 22 62 61 6e  |            "ban|
   +    00000060  6e 65 72 22 3a 20 22 74  68 65 57 72 6f 6e 67 41  |ner": "theWrongA|
   +    00000070  70 70 49 64 22 0a 20 20  20 20 20 20 20 20 20 20  |ppId".          |
   +    00000080  20 20 7d 0a 20 20 20 20  20 20 20 20 20 20 7d 0a  |  }.          }.|
   +    00000090  20 20 20 20 20 20 20 20  7d                       |        }|
       }

Copy link
Contributor

@SyntaxNode SyntaxNode Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Strange that it alters the json encoding. Looks like it's inserting tab formatting.

How about giving https://github.com/mohae/deepcopy a try?

The github.com/getlantern/deepcopy library is not worth referencing, IMHO. Consider writing a helper method instead of importing the library. The following is the full source code of the library:

func Copy(dst interface{}, src interface{}) error {
	if dst == nil {
		return fmt.Errorf("dst cannot be nil")
	}
	if src == nil {
		return fmt.Errorf("src cannot be nil")
	}
	bytes, err := json.Marshal(src)
	if err != nil {
		return fmt.Errorf("Unable to marshal src: %s", err)
	}
	err = json.Unmarshal(bytes, dst)
	if err != nil {
		return fmt.Errorf("Unable to unmarshal into dst: %s", err)
	}
	return nil
}


// Assert Imps separately
assertNoImpsDataRace(t, impsBefore, impsAfter)
}

// assertNoImpsDataRace compares the contents of the reference fields found in the original openrtb.Imp objects to
// their original values to make sure they were not modified and we are not incurring in data races.
func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb.Imp, impsAfter []openrtb.Imp) {
t.Helper()

assert.Len(t, impsAfter, len(impsBefore), "Original []Imp array was modified and length is not equal to original after MakeRequests was called")

// Assert no data races occured in individual Imp elements
for i := 0; i < len(impsBefore); i++ {
assert.Equal(t, impsBefore[i].Banner, impsAfter[i].Banner, "Data race in bidRequest.Imp[%d].Banner field", i)
assert.Equal(t, impsBefore[i].Video, impsAfter[i].Video, "Data race in bidRequest.Imp[%d].Video field", i)
assert.Equal(t, impsBefore[i].Audio, impsAfter[i].Audio, "Data race in bidRequest.Imp[%d].Audio field", i)
assert.Equal(t, impsBefore[i].Native, impsAfter[i].Native, "Data race in bidRequest.Imp[%d].Native field", i)
assert.Equal(t, impsBefore[i].PMP, impsAfter[i].PMP, "Data race in bidRequest.Imp[%d].PMP field", i)
assert.Equal(t, impsBefore[i].Secure, impsAfter[i].Secure, "Data race in bidRequest.Imp[%d].Secure field", i)

assert.ElementsMatch(t, impsBefore[i].Metric, impsAfter[i].Metric, "Data race in bidRequest.Imp[%d].[]Metric array", i)
assert.ElementsMatch(t, impsBefore[i].IframeBuster, impsAfter[i].IframeBuster, "Data race in bidRequest.Imp[%d].[]IframeBuster array", i)
}
}
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 @@ -21,6 +21,7 @@ require (
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b
github.com/hashicorp/hcl v1.0.0 // indirect
github.com/influxdata/influxdb v1.6.1
github.com/jinzhu/copier v0.2.8
github.com/julienschmidt/httprouter v1.1.0
github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 // indirect
github.com/kr/pretty v0.2.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI=
github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
github.com/influxdata/influxdb v1.6.1 h1:OseoBlzI5ftNI/bczyxSWq6PKRCNEeiXvyWP/wS5fB0=
github.com/influxdata/influxdb v1.6.1/go.mod h1:qZna6X/4elxqT3yI9iZYdZrWWdeFOOprn86kgg4+IzY=
github.com/jinzhu/copier v0.2.8 h1:N8MbL5niMwE3P4dOwurJixz5rMkKfujmMRFmAanSzWE=
github.com/jinzhu/copier v0.2.8/go.mod h1:24xnZezI2Yqac9J61UC6/dG/k76ttpq0DdJI3QmUvro=
github.com/julienschmidt/httprouter v1.1.0 h1:7wLdtIiIpzOkC9u6sXOozpBauPdskj3ru4EI5MABq68=
github.com/julienschmidt/httprouter v1.1.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w=
github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 h1:uC1QfSlInpQF+M0ao65imhwqKnz3Q2z/d8PWZRMQvDM=
Expand Down