Skip to content

Commit

Permalink
Core & currency module: fix bug where bid responses are collected aft…
Browse files Browse the repository at this point in the history 
…er auction ends (#10558)

* Core & currency module: fix bug where bid responses are collected after auction ends

* hook fn does not need to be a hook
  • Loading branch information
@dgirardi
dgirardi committed Oct 4, 2023
1 parent 1155fb9 commit 5cb4bcb
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 150 deletions.
23 changes: 9 additions & 14 deletions modules/currency.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,7 @@ export var currencyRates = {};
var bidderCurrencyDefault = {};
var defaultRates;

export const ready = (() => {
let ctl;
function reset() {
ctl = defer();
}
reset();
return {done: () => ctl.resolve(), reset, promise: () => ctl.promise}
})();
export let responseReady = defer();

/**
* Configuration function for currency
Expand Down Expand Up @@ -137,6 +130,7 @@ function initCurrency(url) {
// Adding conversion function to prebid global for external module and on page use
getGlobal().convertCurrency = (cpm, fromCurrency, toCurrency) => parseFloat(cpm) * getCurrencyConversion(fromCurrency, toCurrency);
getHook('addBidResponse').before(addBidResponseHook, 100);
getHook('responsesReady').before(responsesReadyHook);

// call for the file if we haven't already
if (needToCallForCurrencyFile) {
Expand All @@ -150,26 +144,23 @@ function initCurrency(url) {
conversionCache = {};
currencyRatesLoaded = true;
processBidResponseQueue();
ready.done();
} catch (e) {
errorSettingsRates('Failed to parse currencyRates response: ' + response);
}
},
error: function (...args) {
errorSettingsRates(...args);
ready.done();
}
}
);
} else {
ready.done();
}
}

function resetCurrency() {
logInfo('Uninstalling addBidResponse decorator for currency module', arguments);

getHook('addBidResponse').getHooks({hook: addBidResponseHook}).remove();
getHook('responsesReady').getHooks({hook: responsesReadyHook}).remove();
delete getGlobal().convertCurrency;

adServerCurrency = 'USD';
Expand All @@ -179,6 +170,11 @@ function resetCurrency() {
needToCallForCurrencyFile = true;
currencyRates = {};
bidderCurrencyDefault = {};
responseReady = defer();
}

function responsesReadyHook(next, ready) {
next(ready.then(() => responseReady.promise));
}

export const addBidResponseHook = timedBidResponseHook('currency', function addBidResponseHook(fn, adUnitCode, bid, reject) {
Expand Down Expand Up @@ -215,15 +211,14 @@ export const addBidResponseHook = timedBidResponseHook('currency', function addB
bidResponseQueue.push(wrapFunction(fn, this, [adUnitCode, bid, reject]));
if (!currencySupportEnabled || currencyRatesLoaded) {
processBidResponseQueue();
} else {
fn.untimed.bail(ready.promise());
}
});

function processBidResponseQueue() {
while (bidResponseQueue.length > 0) {
(bidResponseQueue.shift())();
}
responseReady.resolve()
}

function wrapFunction(fn, context, params) {
Expand Down
41 changes: 11 additions & 30 deletions src/auction.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,14 @@ export const addBidResponse = hook('sync', function(adUnitCode, bid, reject) {
this.dispatch.call(null, adUnitCode, bid);
}, 'addBidResponse');

/**
* Delay hook for adapter responses.
*
* `ready` is a promise; auctions wait for it to resolve before closing. Modules can hook into this
* to delay the end of auctions while they perform initialization that does not need to delay their start.
*/
export const responsesReady = hook('sync', (ready) => ready, 'responsesReady');

export const addBidderRequests = hook('sync', function(bidderRequests) {
this.dispatch.call(this.context, bidderRequests);
}, 'addBidderRequests');
Expand All @@ -425,32 +433,6 @@ export function auctionCallbacks(auctionDone, auctionInstance, {index = auctionM
let allAdapterCalledDone = false;
let bidderRequestsDone = new Set();
let bidResponseMap = {};
const ready = {};

function waitFor(requestId, result) {
if (ready[requestId] == null) {
ready[requestId] = GreedyPromise.resolve();
}
ready[requestId] = ready[requestId].then(() => GreedyPromise.resolve(result).catch(() => {}))
}

function guard(bidderRequest, fn) {
let timeout = bidderRequest.timeout;
if (timeout == null || timeout > auctionInstance.getTimeout()) {
timeout = auctionInstance.getTimeout();
}
const timeRemaining = auctionInstance.getAuctionStart() + timeout - Date.now();
const wait = ready[bidderRequest.bidderRequestId];
const orphanWait = ready['']; // also wait for "orphan" responses that are not associated with any request
if ((wait != null || orphanWait != null) && timeRemaining > 0) {
GreedyPromise.race([
GreedyPromise.timeout(timeRemaining),
GreedyPromise.resolve(orphanWait).then(() => wait)
]).then(fn);
} else {
fn();
}
}

function afterBidAdded() {
outstandingBidsAdded--;
Expand Down Expand Up @@ -525,8 +507,7 @@ export function auctionCallbacks(auctionDone, auctionInstance, {index = auctionM
return {
addBidResponse: (function () {
function addBid(adUnitCode, bid) {
const bidderRequest = index.getBidderRequest(bid);
waitFor((bidderRequest && bidderRequest.bidderRequestId) || '', addBidResponse.call({
addBidResponse.call({
dispatch: acceptBidResponse,
}, adUnitCode, bid, (() => {
let rejected = false;
Expand All @@ -536,13 +517,13 @@ export function auctionCallbacks(auctionDone, auctionInstance, {index = auctionM
rejected = true;
}
}
})()));
})())
}
addBid.reject = rejectBidResponse;
return addBid;
})(),
adapterDone: function () {
guard(this, adapterDone.bind(this))
responsesReady(GreedyPromise.resolve()).finally(() => adapterDone.call(this));
}
}
}
Expand Down
120 changes: 29 additions & 91 deletions test/spec/auctionmanager_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
adjustBids,
getMediaTypeGranularity,
getPriceByGranularity,
addBidResponse, resetAuctionState
addBidResponse, resetAuctionState, responsesReady
} from 'src/auction.js';
import CONSTANTS from 'src/constants.json';
import * as auctionModule from 'src/auction.js';
Expand Down Expand Up @@ -1803,116 +1803,54 @@ describe('auctionmanager.js', function () {
sinon.assert.calledWith(auction.addBidReceived, sinon.match({cpm: 1.23}));
})

describe('when addBidResponse hook returns promises', () => {
let resolvers, callbacks, bids;
describe('when responsesReady defers', () => {
let resolve, reject, promise, callbacks, bids;

function hook(next, ...args) {
next.bail(new Promise((resolve, reject) => {
resolvers.resolve.push(resolve);
resolvers.reject.push(reject);
}).finally(() => next(...args)));
function hook(next, ready) {
next(ready.then(() => promise));
}

function invokeCallbacks() {
bids.forEach((bid) => callbacks.addBidResponse(ADUNIT_CODE, bid));
bidRequests.forEach(bidRequest => callbacks.adapterDone.call(bidRequest));
}
before(() => {
responsesReady.before(hook);
});

function delay(ms = 0) {
return new Promise((resolve) => {
setTimeout(resolve, ms)
});
}
after(() => {
responsesReady.getHooks({hook}).remove();
});

beforeEach(() => {
// eslint-disable-next-line promise/param-names
promise = new Promise((rs, rj) => {
resolve = rs;
reject = rj;
});
bids = [
mockBid({bidderCode: BIDDER_CODE1}),
mockBid({bidderCode: BIDDER_CODE})
]
bidRequests = bids.map((b) => mockBidRequest(b));
resolvers = {resolve: [], reject: []};
addBidResponse.before(hook);
callbacks = auctionCallbacks(doneSpy, auction);
Object.assign(auction, {
addNoBid: sinon.spy()
});
});

afterEach(() => {
addBidResponse.getHooks({hook: hook}).remove();
});

it('should wait for bids without a request bids before calling auctionDone', () => {
callbacks.addBidResponse(ADUNIT_CODE, Object.assign(mockBid(), {requestId: null}));
invokeCallbacks();
resolvers.resolve.slice(1, 3).forEach((fn) => fn());
return delay().then(() => {
expect(doneSpy.called).to.be.false;
resolvers.resolve[0]();
return delay();
}).then(() => {
expect(doneSpy.called).to.be.true;
});
});

Object.entries({
'all succeed': ['resolve', 'resolve'],
'some fail': ['resolve', 'reject'],
'all fail': ['reject', 'reject']
}).forEach(([test, results]) => {
describe(`(and ${test})`, () => {
it('should wait for them to complete before calling auctionDone', () => {
invokeCallbacks();
return delay().then(() => {
expect(doneSpy.called).to.be.false;
expect(auction.addNoBid.called).to.be.false;
resolvers[results[0]][0]();
return delay();
}).then(() => {
expect(doneSpy.called).to.be.false;
expect(auction.addNoBid.called).to.be.false;
resolvers[results[1]][1]();
return delay();
}).then(() => {
expect(doneSpy.called).to.be.true;
});
});
'resolve': () => resolve(),
'reject': () => reject(),
}).forEach(([t, resolver]) => {
it(`should wait for responsesReady to ${t} before calling auctionDone`, (done) => {
bidRequests.forEach(bidRequest => callbacks.adapterDone.call(bidRequest));
setTimeout(() => {
sinon.assert.notCalled(doneSpy);
resolver();
setTimeout(() => {
sinon.assert.called(doneSpy);
done();
})
})
});
});

Object.entries({
bidder: (timeout) => {
bidRequests.forEach((r) => r.timeout = timeout);
auction.getTimeout = () => timeout + 10000
},
auction: (timeout) => {
auction.getTimeout = () => timeout;
bidRequests.forEach((r) => r.timeout = timeout + 10000)
}
}).forEach(([test, setTimeout]) => {
it(`should respect ${test} timeout if they never complete`, () => {
const start = Date.now() - 2900;
auction.getAuctionStart = () => start;
setTimeout(3000);
invokeCallbacks();
return delay().then(() => {
expect(doneSpy.called).to.be.false;
return delay(100);
}).then(() => {
expect(doneSpy.called).to.be.true;
});
});

it(`should not wait if ${test} has already timed out`, () => {
const start = Date.now() - 2000;
auction.getAuctionStart = () => start;
setTimeout(1000);
invokeCallbacks();
return delay().then(() => {
expect(doneSpy.called).to.be.true;
});
});
})
});

describe('when bids are rejected', () => {
Expand Down
26 changes: 11 additions & 15 deletions test/spec/modules/currency_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
addBidResponseHook,
currencySupportEnabled,
currencyRates,
ready
responseReady
} from 'modules/currency.js';
import {createBid} from '../../../src/bidfactory.js';
import CONSTANTS from '../../../src/constants.json';
Expand All @@ -32,7 +32,6 @@ describe('currency', function () {

beforeEach(function () {
fakeCurrencyFileServer = server;
ready.reset();
});

afterEach(function () {
Expand Down Expand Up @@ -321,29 +320,26 @@ describe('currency', function () {

fakeCurrencyFileServer.respondWith(JSON.stringify(getCurrencyRates()));

var bid = { 'cpm': 1, 'currency': 'USD' };
const bid = { 'cpm': 1, 'currency': 'USD' };

setConfig({ 'adServerCurrency': 'JPY' });

var marker = false;
let promiseResolved = false;
let responseAdded = false;
let isReady = false;
responseReady.promise.then(() => { isReady = true });

addBidResponseHook(Object.assign(function() {
marker = true;
}, {
bail: function (promise) {
promise.then(() => promiseResolved = true);
}
responseAdded = true;
}), 'elementId', bid);

expect(marker).to.equal(false);

setTimeout(() => {
expect(promiseResolved).to.be.false;
expect(responseAdded).to.equal(false);
expect(isReady).to.equal(false);
fakeCurrencyFileServer.respond();

setTimeout(() => {
expect(marker).to.equal(true);
expect(promiseResolved).to.be.true;
expect(responseAdded).to.equal(true);
expect(isReady).to.equal(true);
done();
});
});
Expand Down

0 comments on commit 5cb4bcb

Please sign in to comment.