-
Notifications
You must be signed in to change notification settings - Fork 832
Triplelift Native: fix panic #3825
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.
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
Conversation
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report
|
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report
|
@@ -50,12 +50,6 @@ func processImp(imp *openrtb2.Imp, request *openrtb2.BidRequest) error { | |||
// get the triplelift extension | |||
var ext ExtImp | |||
var tlext openrtb_ext.ExtImpTriplelift | |||
var siteCopy openrtb2.Site | |||
var extData ExtImpData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying request.Site
into siteCopy
and ext.Data
into extData
created overhead and, given that the processImp
function is only interested in reading and not overwriting these values, no shared memory issues will come if we just use the originals. Let me know if you agree with their removal.
We've opened two PRs for the same problem. Closing as we already have #3824. These questions have been asked offline. |
This pull request adds an extra
nil
check to the logic recently added in #3745 as it was triggering panics when anil
value ofsiteCopy.Publisher
would get dereferenced in line 78 ofadapters/triplelift_native/triplelift_native.go
.@triplelift, we were wondering if:
Do you agree with this change?
Given that the
imp.TagID
can now come from eitherext.Data.TagCode
ortlext.InvCode
, do we still want to return error in line 64 or should we remove?If
request.Site
isnil
, are we interested in using theApp.Publisher.Domain
instead? FunctioneffectivePubID
downstream makes use ofApp.Publisher
values. ShouldprocessImp
useApp.Publisher.Domain
if available?