Richaudience: Shared memory fix #2710
Conversation
| @@ -52,10 +52,18 @@ func (a *adapter) MakeRequests(request *openrtb2.BidRequest, requestInfo *adapte | |||
| } | |||
|
|
|||
| if request.App != nil { | |||
|
|
|||
There was a problem hiding this comment.
Nitpick: Please remove the leading white space here and for the Site check.
| } | ||
|
|
||
| b := &adapters.TypedBid{ | ||
| Bid: &seatBid.Bid[i], | ||
| Bid: &bidResp.SeatBid[j].Bid[i], | ||
| BidType: bidType, | ||
| } |
There was a problem hiding this comment.
Why are these changes necessary for the bid processing loop? I'm not seeing the issue with the original code.
There was a problem hiding this comment.
There are no issues with this. We discussed this with the team and decided this is the right way to do this, similar to how we access Bid by index. I can revert it back.
There was a problem hiding this comment.
I would recommend to revert this change. There's nothing wrong with the original code and this change is unrelated to the memory corruption.
Arguably, seatBid.Bid[i] is preferred over bidResp.SeatBid[j].Bid[i].
There was a problem hiding this comment.
request.Device.IP also has a shared memory violation issue. @guscarreon these lines of code are covered by tests, why did the shared memory assertions pass here?
This does not appear to be an infrastructure issue; it's the tests. Let's update the We should make sure that similar adjustments are made to the tests to detect shared memory issues with the device object. |
Good catch about Device.Ip! |
|
@richaudience Can you please confirm (or not) you always expect only 1 impression per request? |
* Shared memory fix * Minor reference usage fix * Removed empty lines * Code review feedback addressed * Added nil check for req.device
Adapters receive a shallow copy of a request In MakeBids. It means this request has shared pointer values: site, app and other pointer structures are shared between all adapters in request. In order to modify pointer structures you need to create own copy of it and set it back to request.
The reason why previous unit tests didn't show it is because
imp.tagidwas empty and resultapp/site.keywordsdidn't change.Please review. @richaudience