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.

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

Sovrn adapter - create no-bid responses for those placements where sovrn doesn't give a response. #163

Closed
wants to merge 6 commits into from

Conversation

baragona
Copy link
Contributor

If you request 5 ad units, and there are no bids, prebid.js should detect that no more bids are going to come back and return immediately.

If you request 5 and get 3 back, the same thing should apply, create blank bid responses for the 2 with no-bids, and prebid.js should detect that there are no more possible bids that could come back.

I don't think the expectedCount system could be used here because if it was set to 1, prebid would call back after the first bid was processed even if there were more bids available.

This change was made to correct the case where multiple ad units were requested of Sovrn, and a no-bid response caused prebid to wait for other bids to come back, but they never would, so it would wait for the full timeout amount unnecessarily.

Hopefully this makes sense and doesn't introduce new bugs. Let me know if I can help explain it better. Thanks.

@pdramos1
Copy link
Collaborator

pdramos1 commented Feb 1, 2016

@ojotoxy , hey Just looking for some clarifications about what this pull request is solving. From my understanding the bidmanager simply doesn't find the responses that match and moves on. (I could be wrong on this though)

@mkendall07
Copy link
Member

@ojotoxy It would help greatly if you included a test file that demonstrates the bad behavior. Thanks!

@baragona
Copy link
Contributor Author

baragona commented Feb 1, 2016

My understanding is that the bid manager wants to see addBidResponse called the same number of times as expectedBidsCount, before executing the callback. Sovrn adapter doesn't set expectedBidsCount, so it will be set to the default (from adaptermanager.js):

            // if the bidder didn't explicitly set the number of bids
            // expected, default to the number of bids passed into the bidder

Multiple Sovrn bid requests are merged into a single call to sovrn. When sovrn encounters an error or doesn't bid on anything, they don't sent an explicit no-bid response for each bid request. Before this PR change, the sovrn adapter called addBidResponse only 1 time in this case.

Thus addBidResponse may only be called 1 time. But bidmanager expects it to be called more times, so it does not fire the callback immediately, instead, it waits for the timeout to occur.

I am pretty swamped with work so I can't make an example test today... (In any case its challenging to make a test that relies on an external server and needs a per-user tag id, etc.) But at some point in the future I could make a test case if it is helpful.

I've been running this code in production for a couple weeks now and it seems to work well.

@baragona
Copy link
Contributor Author

baragona commented Feb 1, 2016

It seems like this same issue could affect some other adapters too? Maybe the simplest way to deal with this would be to add a function to the bidmanager that the adapter can use to say: "I have received all the responses I am going to get"

@pdramos1
Copy link
Collaborator

pdramos1 commented Feb 1, 2016

OH, I see. That makes sense. I can work on making a test to verify this works etc. and we should be able to move forward fairly quickly.

@mkendall07
Copy link
Member

Thanks for reviewing @pdramos1

@mkendall07
Copy link
Member

@ojotoxy - can you remove changes from criteo.js from this PR? Thanks

@baragona
Copy link
Contributor Author

baragona commented Feb 8, 2016

New PR is here: (github is confusing)

#183

@baragona baragona closed this Feb 8, 2016
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants