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

Fix timeout issues #699 #700

Closed
wants to merge 6 commits into from
Closed

Fix timeout issues #699 #700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
03d050a
Fix timeout issues #699
dugwood Oct 12, 2016
0541c63
Merge branch 'master' of ssh://github.com/prebid/Prebid.js
dugwood Oct 13, 2016
e47efec
Fix bidsBackAll that returns true because removeComplete is bugged
dugwood Oct 13, 2016
5eaa3a6
Merge branch 'master' of ssh://github.com/prebid/Prebid.js
dugwood Oct 14, 2016
c73cbd6
Fix for handling old bids
dugwood Oct 14, 2016
fd9a6e7
Handle tests
dugwood Oct 14, 2016
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
52 changes: 29 additions & 23 deletions src/bidmanager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var objectType_function = 'function';
var externalCallbackByAdUnitArr = [];
var externalCallbackArr = [];
var externalOneTimeCallback = null;
var externalOneTimeCallbackTimer = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok for now, though I think we could consider a callbackHandler object with callback and timer function properties and externalCallback... array properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed this in #735

var _granularity = CONSTANTS.GRANULARITY_OPTIONS.MEDIUM;
var defaultBidderSettingsMap = {};

Expand Down Expand Up @@ -40,29 +41,27 @@ function getBidders(bid) {
return bid.bidder;
}

function bidsBackAdUnit(adUnitCode) {
let requested = $$PREBID_GLOBAL$$.adUnits.find(unit => unit.code === adUnitCode);
if (requested) {requested = requested.bids.length;}
const received = $$PREBID_GLOBAL$$._bidsReceived.filter(bid => bid.adUnitCode === adUnitCode).length;
return requested === received;
}

function add(a, b) {
return a + b;
}

function bidsBackAll() {
const requested = $$PREBID_GLOBAL$$._bidsRequested.map(bidSet => bidSet.bids.length).reduce(add, 0);
const received = $$PREBID_GLOBAL$$._bidsReceived.length;
function bidsBack(adUnitCodes) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my testing this is called twice from addBidResponse, once with the adUnitCode of the bid response and once without arguments. In the latter case requested and received both evaluate to 0 which returns true from this function which in turn invokes the callback earlier than expected and repeatedly for each call to bidmanager.addBidResponse.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of combining the two bidsBack... functions. If adUnitCodes is not defined it could be derived from pbjs.adUnits, and a .filter function applied if an adUnitCode is supplied to bidsBack function.

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 see this issue with both zero values... But I'll look for a fix in received, which is the culprit in the case of two auctions (the second time the requested isn't filtered correctly).

adUnitCodes = adUnitCodes || $$PREBID_GLOBAL$$._adUnitCodes;
const requested = $$PREBID_GLOBAL$$._bidsRequested.map(bidSet => {
return bidSet.bids.filter(bid => adUnitCodes.includes(bid.placementCode)).length;
}).reduce(add, 0);
const received = $$PREBID_GLOBAL$$._bidsReceived.filter(bid => adUnitCodes.includes(bid.adUnitCode)).length;
return requested === received;
}

exports.bidsBackAll = function() {
return bidsBackAll();
exports.bidsBack = function() {
return bidsBack();
};

function getBidSetForBidder(bidder) {
return $$PREBID_GLOBAL$$._bidsRequested.find(bidSet => bidSet.bidderCode === bidder) || { start: null, requestId: null };
function getBidSetForBidder(bidder, adUnitCode) {
return $$PREBID_GLOBAL$$._bidsRequested.find(bidSet => {
return bidSet.bids.filter(bid => bid.bidder === bidder && bid.placementCode === adUnitCode).length > 0;
}) || { start: null, requestId: null };
}

/*
Expand All @@ -71,10 +70,11 @@ function getBidSetForBidder(bidder) {
exports.addBidResponse = function (adUnitCode, bid) {
if (bid) {

const bidSet = getBidSetForBidder(bid.bidderCode, adUnitCode);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be expressed as
const { requestId, start } = getBidSetForBidder(bid.bidderCode, adUnitCode);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Object.assign(bid, {
requestId: getBidSetForBidder(bid.bidderCode).requestId,
requestId: bidSet.requestId,
responseTimestamp: timestamp(),
requestTimestamp: getBidSetForBidder(bid.bidderCode).start,
requestTimestamp: bidSet.start,
cpm: bid.cpm || 0,
bidder: bid.bidderCode,
adUnitCode
Expand All @@ -83,9 +83,7 @@ exports.addBidResponse = function (adUnitCode, bid) {
bid.timeToRespond = bid.responseTimestamp - bid.requestTimestamp;

if (bid.timeToRespond > $$PREBID_GLOBAL$$.cbTimeout + $$PREBID_GLOBAL$$.timeoutBuffer) {
const timedOut = true;

this.executeCallback(timedOut);
this.executeCallback(true);
}

//emit the bidAdjustment event before bidResponse, so bid response has the adjusted bid value
Expand Down Expand Up @@ -117,11 +115,11 @@ exports.addBidResponse = function (adUnitCode, bid) {
$$PREBID_GLOBAL$$._bidsReceived.push(bid);
}

if (bid && bid.adUnitCode && bidsBackAdUnit(bid.adUnitCode)) {
if (bid && bid.adUnitCode && bidsBack([bid.adUnitCode])) {
triggerAdUnitCallbacks(bid.adUnitCode);
}

if (bidsBackAll()) {
if (bidsBack()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As noted above this can evaluate to true when requested and received both evaluate to 0 .

this.executeCallback();
}
};
Expand Down Expand Up @@ -222,6 +220,11 @@ exports.registerDefaultBidderSetting = function (bidderCode, defaultSetting) {
};

exports.executeCallback = function (timedOut) {
// if there's still a timeout running, clear it now
if (!timedOut && externalOneTimeCallbackTimer) {
clearTimeout(externalOneTimeCallbackTimer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in #735

}

if (externalCallbackArr.called !== true) {
processCallbacks(externalCallbackArr);
externalCallbackArr.called = true;
Expand All @@ -242,6 +245,7 @@ exports.executeCallback = function (timedOut) {
}
finally {
externalOneTimeCallback = null;
externalOneTimeCallbackTimer = false;
$$PREBID_GLOBAL$$.clearAuction();
}
}
Expand Down Expand Up @@ -290,10 +294,12 @@ function groupByPlacement(prev, item, idx, arr) {

/**
* Add a one time callback, that is discarded after it is called
* @param {Function} callback [description]
* @param {Function} callback
* @param timer Timer to clear if callback is triggered before timer time's out
*/
exports.addOneTimeCallback = function (callback) {
exports.addOneTimeCallback = function (callback, timer) {
externalOneTimeCallback = callback;
externalOneTimeCallbackTimer = timer;
};

exports.addCallback = function (id, callback, cbEvent) {
Expand Down
62 changes: 29 additions & 33 deletions src/prebid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var eventValidators = {

$$PREBID_GLOBAL$$._bidsRequested = [];
$$PREBID_GLOBAL$$._bidsReceived = [];
$$PREBID_GLOBAL$$._adUnitCodes = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid another primary data structure here and consider adUnitCodes to be derived from adUnits. The codes are a shorthand way of refreshing bids for a previously defined group of adUnits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is that you can't know what adUnits are currently used in the current auction... But maybe that's because I've got rid of markComplete.

$$PREBID_GLOBAL$$._winningBids = [];
$$PREBID_GLOBAL$$._adsReceived = [];
$$PREBID_GLOBAL$$._sendAllBids = false;
Expand Down Expand Up @@ -262,29 +263,22 @@ function getAllTargeting() {
return targeting;
}

function markComplete(adObject) {
$$PREBID_GLOBAL$$._bidsRequested.filter(request => request.requestId === adObject.requestId)
.forEach(request => request.bids.filter(bid => bid.placementCode === adObject.adUnitCode)
.forEach(bid => bid.complete = true));

$$PREBID_GLOBAL$$._bidsReceived.filter(bid => {
return bid.requestId === adObject.requestId && bid.adUnitCode === adObject.adUnitCode;
}).forEach(bid => bid.complete = true);
}

function removeComplete() {
function removePreviousBids(adUnitCode) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This raises the question of determining when bids are no longer valid. In removePreviousBids here you remove all bids for a placement if that placement is included in the auction about to start. The code that this replaces marks bids complete for a placement if pbjs.renderAd is called for that placement, which means we know the auction was run and the winning ad has been rendered. Those "complete" bids are only removed when a subsequent bid request is about to go out. This gives publishers as open a window as possible to call the reporting APIs for the most recent auction. Also, renderAd is the only definitive knowledge Prebid has of the ad delivery. There is currently no Prebid handler for an ad render when filled by ad server demand.

As noted here and here and with this PR, the approach results in some unexpected "old" bids. However consider the case that there is nothing invalid about these bids necessarily, and including them in auctions may be desirable. For example, take a given placement auction where the highest bid is $5. As it happens the ad server fills with it's own demand. A subsequent request for bids for the placement returns a winner of $4. The "old" $5 bid is still a valid bid as far as the bidder is concerned. Why should we exclude the bid from the current outgoing auction?

We are already throwing out some bids we perhaps shouldn't. Consider a case where Prebid demand fills a placement. The winner was a $10 bid, with a $5 bid behind it. A second request for bids goes out with a winner of $2. In this case we currently markComplete and removeComplete so the $5 bid is thrown out and not considered in the second auction. Again, as far as the bidder is concerned, that is a valid $5 bid for the placement. Should we throw it out?

let requests = $$PREBID_GLOBAL$$._bidsRequested;
let responses = $$PREBID_GLOBAL$$._bidsReceived;

requests.map(request => request.bids
.filter(bid => bid.complete))
.forEach(request => requests.splice(requests.indexOf(request), 1));

responses.filter(bid => bid.complete).forEach(bid => responses.splice(responses.indexOf(bid), 1));
requests.filter(request => {
request.bids.filter(bid => {
// bid has the placementCode, remove it
if (bid.placementCode === adUnitCode) {
return request.bids.splice(request.bids.indexOf(bid), 1);
}
});
// if there's no more bids, then the request can be removed
return request.bids.length === 0;
}).forEach(request => requests.splice(requests.indexOf(request), 1));

// also remove bids that have an empty or error status so known as not pending for render
responses.filter(bid => bid.getStatusCode && bid.getStatusCode() === 2)
.forEach(bid => responses.splice(responses.indexOf(bid), 1));
responses.filter(bid => bid.placementCode === adUnitCode).forEach(bid => responses.splice(responses.indexOf(bid), 1));
}

//////////////////////////////////
Expand Down Expand Up @@ -427,7 +421,7 @@ $$PREBID_GLOBAL$$.setTargetingForGPTAsync = function () {
*/
$$PREBID_GLOBAL$$.allBidsAvailable = function () {
utils.logInfo('Invoking $$PREBID_GLOBAL$$.allBidsAvailable', arguments);
return bidmanager.bidsBackAll();
return bidmanager.bidsBack();
};

/**
Expand All @@ -449,8 +443,6 @@ $$PREBID_GLOBAL$$.renderAd = function (doc, id) {
//emit 'bid won' event here
events.emit(BID_WON, adObject);

// mark bid requests and responses for this placement in this auction as "complete"
markComplete(adObject);
var height = adObject.height;
var width = adObject.width;
var url = adObject.adUrl;
Expand Down Expand Up @@ -526,9 +518,10 @@ $$PREBID_GLOBAL$$.clearAuction = function() {
$$PREBID_GLOBAL$$.requestBids = function ({ bidsBackHandler, timeout, adUnits, adUnitCodes }) {
const cbTimeout = $$PREBID_GLOBAL$$.cbTimeout = timeout || $$PREBID_GLOBAL$$.bidderTimeout;
adUnits = adUnits || $$PREBID_GLOBAL$$.adUnits;
adUnitCodes = adUnitCodes || [];

// if specific adUnitCodes filter adUnits for those codes
if (adUnitCodes && adUnitCodes.length) {
if (adUnitCodes.length) {
adUnits = adUnits.filter(adUnit => adUnitCodes.includes(adUnit.code));
}

Expand All @@ -543,30 +536,33 @@ $$PREBID_GLOBAL$$.requestBids = function ({ bidsBackHandler, timeout, adUnits, a

if (auctionRunning) {
bidRequestQueue.push(() => {
$$PREBID_GLOBAL$$.requestBids({ bidsBackHandler, cbTimeout, adUnits });
$$PREBID_GLOBAL$$.requestBids({ bidsBackHandler, timeout: cbTimeout, adUnits, adUnitCodes });
});
return;
} else {
auctionRunning = true;
removeComplete();
}

if (typeof bidsBackHandler === objectType_function) {
bidmanager.addOneTimeCallback(bidsBackHandler);
auctionRunning = true;
for (let i = 0; i < adUnitCodes.length; i++) {
removePreviousBids(adUnitCodes[i]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

As noted above we should avoid promoting adUnitCodes as a primary data structure. The initial call to pbjs.requestBids must supply adUnits. Thereafter bids can be requested with adUnitCodes but only for adUnits already defined. In this block, for example, removePreviousBids will only be invoked if there are adUnitCodes, which may not be the case. See the 'No adUnits configured..' log message on line 552. adUnits are assumed to be present, and if adUnitCodes are present they are only used to filter adUnits.

}
$$PREBID_GLOBAL$$._adUnitCodes = adUnitCodes;

utils.logInfo('Invoking $$PREBID_GLOBAL$$.requestBids', arguments);

if (!adUnits || adUnits.length === 0) {
utils.logMessage('No adUnits configured. No bids requested.');
if (typeof bidsBackHandler === objectType_function) {
bidmanager.addOneTimeCallback(bidsBackHandler);
}
bidmanager.executeCallback();
return;
}

//set timeout for all bids
const timedOut = true;
const timeoutCallback = bidmanager.executeCallback.bind(bidmanager, timedOut);
setTimeout(timeoutCallback, cbTimeout);
const timeoutCallback = bidmanager.executeCallback.bind(bidmanager, true);
const timer = setTimeout(timeoutCallback, cbTimeout);
if (typeof bidsBackHandler === objectType_function) {
bidmanager.addOneTimeCallback(bidsBackHandler, timer);
}

adaptermanager.callBids({ adUnits, adUnitCodes, cbTimeout });
};
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,13 @@ export function getSlotTargeting() {
};
}

export function getAdUnitCodes() {
return [
"/19968336/header-bid-tag-0",
"/19968336/header-bid-tag1"
];
};

export function getAdUnits() {
return [
{
Expand Down
7 changes: 5 additions & 2 deletions test/spec/unit/pbjs_api_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
getBidResponsesFromAPI,
getTargetingKeys,
getTargetingKeysBidLandscape,
getAdUnitCodes,
getAdUnits
} from 'test/fixtures/fixtures';

Expand All @@ -25,13 +26,15 @@ var config = require('test/fixtures/config.json');
$$PREBID_GLOBAL$$ = $$PREBID_GLOBAL$$ || {};
$$PREBID_GLOBAL$$._bidsRequested = getBidRequests();
$$PREBID_GLOBAL$$._bidsReceived = getBidResponses();
$$PREBID_GLOBAL$$._adUnitCodes = getAdUnitCodes();
$$PREBID_GLOBAL$$.adUnits = getAdUnits();

function resetAuction() {
$$PREBID_GLOBAL$$._sendAllBids = false;
$$PREBID_GLOBAL$$.clearAuction();
$$PREBID_GLOBAL$$._bidsRequested = getBidRequests();
$$PREBID_GLOBAL$$._bidsReceived = getBidResponses();
$$PREBID_GLOBAL$$._adUnitCodes = getAdUnitCodes();
$$PREBID_GLOBAL$$.adUnits = getAdUnits();

}
Expand Down Expand Up @@ -432,11 +435,11 @@ describe('Unit: Prebid Module', function () {

describe('allBidsAvailable', function () {
it('should call bidmanager.allBidsBack', function () {
var spyAllBidsBack = sinon.spy(bidmanager, 'bidsBackAll');
var spyAllBidsBack = sinon.spy(bidmanager, 'bidsBack');

$$PREBID_GLOBAL$$.allBidsAvailable();
assert.ok(spyAllBidsBack.called, 'called bidmanager.allBidsBack');
bidmanager.bidsBackAll.restore();
bidmanager.bidsBack.restore();
});
});

Expand Down