diff --git a/modules/currency.js b/modules/currency.js index 5f7add764ad..5f27e49798a 100644 --- a/modules/currency.js +++ b/modules/currency.js @@ -20,6 +20,25 @@ export var currencyRates = {}; var bidderCurrencyDefault = {}; var defaultRates; +export const ready = (() => { + let isDone, resolver, promise; + function reset() { + isDone = false; + resolver = null; + promise = new Promise((resolve) => { + resolver = resolve; + if (isDone) resolve(); + }) + } + function done() { + isDone = true; + if (resolver != null) { resolver() } + } + reset(); + + return {done, reset, promise: () => promise} +})(); + /** * Configuration function for currency * @param {string} [config.adServerCurrency = 'USD'] @@ -138,11 +157,15 @@ function initCurrency(url) { logInfo('currencyRates set to ' + JSON.stringify(currencyRates)); currencyRatesLoaded = true; processBidResponseQueue(); + ready.done(); } catch (e) { errorSettingsRates('Failed to parse currencyRates response: ' + response); } }, - error: errorSettingsRates + error: function (...args) { + errorSettingsRates(...args); + ready.done(); + } } ); } @@ -197,6 +220,8 @@ export function addBidResponseHook(fn, adUnitCode, bid) { bidResponseQueue.push(wrapFunction(fn, this, [adUnitCode, bid])); if (!currencySupportEnabled || currencyRatesLoaded) { processBidResponseQueue(); + } else { + fn.bail(ready.promise()); } } diff --git a/src/auction.js b/src/auction.js index 175f08439d1..2172a865b62 100644 --- a/src/auction.js +++ b/src/auction.js @@ -250,10 +250,7 @@ export function newAuction({adUnits, adUnitCodes, callback, cbTimeout, labels, a let callbacks = auctionCallbacks(auctionDone, this); adapterManager.callBids(_adUnits, bidRequests, function(...args) { - addBidResponse.apply({ - dispatch: callbacks.addBidResponse, - bidderRequest: this - }, args) + callbacks.addBidResponse.apply(this, args); }, callbacks.adapterDone, { request(source, origin) { increment(outstandingRequests, origin); @@ -344,6 +341,7 @@ export function newAuction({adUnits, adUnitCodes, callback, cbTimeout, labels, a addWinningBid, setBidTargeting, getWinningBids: () => _winningBids, + getAuctionStart: () => _auctionStart, getTimeout: () => _timeout, getAuctionId: () => _auctionId, getAuctionStatus: () => _auctionStatus, @@ -355,8 +353,12 @@ export function newAuction({adUnits, adUnitCodes, callback, cbTimeout, labels, a } } -export const addBidResponse = hook('async', function(adUnitCode, bid) { - this.dispatch.call(this.bidderRequest, adUnitCode, bid); +/** + * addBidResponse may return a Promise; if it does, the auction will attempt to + * wait for it to complete (successfully or not) before closing. + */ +export const addBidResponse = hook('sync', function(adUnitCode, bid) { + return this.dispatch.call(this.bidderRequest, adUnitCode, bid); }, 'addBidResponse'); export const addBidderRequests = hook('sync', function(bidderRequests) { @@ -374,6 +376,32 @@ export function auctionCallbacks(auctionDone, auctionInstance) { let allAdapterCalledDone = false; let bidderRequestsDone = new Set(); let bidResponseMap = {}; + const ready = {}; + + function waitFor(bidderRequest, result) { + const id = bidderRequest.bidderRequestId; + if (ready[id] == null) { + ready[id] = Promise.resolve(); + } + ready[id] = ready[id].then(() => Promise.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]; + if (wait != null && timeRemaining > 0) { + Promise.race([ + new Promise((resolve) => setTimeout(resolve, timeRemaining)), + wait + ]).then(fn); + } else { + fn(); + } + } function afterBidAdded() { outstandingBidsAdded--; @@ -382,7 +410,7 @@ export function auctionCallbacks(auctionDone, auctionInstance) { } } - function addBidResponse(adUnitCode, bid) { + function handleBidResponse(adUnitCode, bid) { let bidderRequest = this; bidResponseMap[bid.requestId] = true; @@ -429,8 +457,15 @@ export function auctionCallbacks(auctionDone, auctionInstance) { } return { - addBidResponse, - adapterDone + addBidResponse: function (...args) { + waitFor(this, addBidResponse.apply({ + dispatch: handleBidResponse, + bidderRequest: this + }, args)); + }, + adapterDone: function () { + guard(this, adapterDone.bind(this)) + } } } diff --git a/test/helpers/syncPromise.js b/test/helpers/syncPromise.js new file mode 100644 index 00000000000..99361bd716e --- /dev/null +++ b/test/helpers/syncPromise.js @@ -0,0 +1,71 @@ +const orig = {}; +['resolve', 'reject', 'all', 'race', 'allSettled'].forEach((k) => orig[k] = Promise[k].bind(Promise)) + +// Callbacks attached through Promise.resolve(value).then(...) will usually +// not execute immediately even if `value` is immediately available. This +// breaks tests that were written before promises even though they are semantically still valid. +// They can be made to work by making promises quasi-synchronous. + +export function SyncPromise(value, fail = false) { + if (value instanceof SyncPromise) { + return value; + } else if (typeof value === 'object' && typeof value.then === 'function') { + return orig.resolve(value); + } else { + Object.assign(this, { + then: function (cb, err) { + const handler = fail ? err : cb; + if (handler != null) { + return new SyncPromise(handler(value)); + } else { + return this; + } + }, + catch: function (cb) { + if (fail) { + return new SyncPromise(cb(value)) + } else { + return this; + } + }, + finally: function (cb) { + cb(); + return this; + }, + __value: fail ? {status: 'rejected', reason: value} : {status: 'fulfilled', value} + }) + } +} + +Object.assign(SyncPromise, { + resolve: (val) => new SyncPromise(val), + reject: (val) => new SyncPromise(val, true), + race: (promises) => promises.find((p) => p instanceof SyncPromise) || orig.race(promises), + allSettled: (promises) => { + if (promises.every((p) => p instanceof SyncPromise)) { + return new SyncPromise(promises.map((p) => p.__value)) + } else { + return orig.allSettled(promises); + } + }, + all: (promises) => { + if (promises.every((p) => p instanceof SyncPromise)) { + return SyncPromise.allSettled(promises).then((result) => { + const err = result.find((r) => r.status === 'rejected'); + if (err != null) { + return new SyncPromise(err.reason, true); + } else { + return new SyncPromise(result.map((r) => r.value)) + } + }) + } else { + return orig.all(promises); + } + } +}) + +export function synchronizePromise(sandbox) { + Object.keys(orig).forEach((k) => { + sandbox.stub(window.Promise, k).callsFake(SyncPromise[k]); + }) +} diff --git a/test/spec/auctionmanager_spec.js b/test/spec/auctionmanager_spec.js index 69e34c4a07a..0b335674a6b 100644 --- a/test/spec/auctionmanager_spec.js +++ b/test/spec/auctionmanager_spec.js @@ -3,7 +3,7 @@ import { auctionCallbacks, AUCTION_COMPLETED, adjustBids, - getMediaTypeGranularity, + getMediaTypeGranularity, addBidResponse, } from 'src/auction.js'; import CONSTANTS from 'src/constants.json'; import * as auctionModule from 'src/auction.js'; @@ -14,6 +14,10 @@ import * as store from 'src/videoCache.js'; import * as ajaxLib from 'src/ajax.js'; import find from 'core-js-pure/features/array/find.js'; import { server } from 'test/mocks/xhr.js'; +import {expect} from 'chai'; +import {hook} from '../../src/hook.js'; +import 'src/debugging.js' +import {synchronizePromise} from '../helpers/syncPromise.js'; // some of these tests require debugging hooks to be loaded var assert = require('assert'); @@ -124,6 +128,27 @@ function mockAjaxBuilder() { } describe('auctionmanager.js', function () { + let promiseSandbox; + + before(() => { + // hooks are global and their side effects depend on what has been loaded... not ideal for unit tests + [ + auctionModule.addBidResponse, + auctionModule.addBidderRequests, + auctionModule.bidsBackCallback + ].forEach((h) => h.getHooks().remove()) + hook.ready(); + }); + + beforeEach(() => { + promiseSandbox = sinon.createSandbox(); + synchronizePromise(promiseSandbox); + }); + + afterEach(() => { + promiseSandbox.restore(); + }) + describe('getKeyValueTargetingPairs', function () { const DEFAULT_BID = { cpm: 5.578, @@ -1259,12 +1284,7 @@ describe('auctionmanager.js', function () { let bids = TEST_BIDS; let bidRequests; let doneSpy; - let auction = { - getBidRequests: () => bidRequests, - getAuctionId: () => '1', - addBidReceived: () => true, - getTimeout: () => 1000 - } + let auction; beforeEach(() => { doneSpy = sinon.spy(); @@ -1272,12 +1292,21 @@ describe('auctionmanager.js', function () { cache: { url: 'https://prebid.adnxs.com/pbc/v1/cache' } - }) + }); + const start = Date.now(); + auction = { + getBidRequests: () => bidRequests, + getAuctionId: () => '1', + addBidReceived: () => true, + getTimeout: () => 1000, + getAuctionStart: () => start, + } }); afterEach(() => { doneSpy.resetHistory(); config.resetConfig(); + bidRequests = null; }); it('should call auction done after bid is added to auction for mediaType banner', function () { @@ -1328,19 +1357,114 @@ describe('auctionmanager.js', function () { const responseBody = `{"responses":[{"uuid":"${uuid}"}]}`; server.requests[0].respond(200, { 'Content-Type': 'application/json' }, responseBody); assert.equal(doneSpy.callCount, 1); - }) + }); + + describe('when addBidResponse hook returns promises', () => { + let resolvers, callbacks, bids; + + function hook(next, ...args) { + next.bail(new Promise((resolve, reject) => { + resolvers.resolve.push(resolve); + resolvers.reject.push(reject); + }).finally(() => next(...args))); + } + + function invokeCallbacks() { + bids.forEach((bid, i) => callbacks.addBidResponse.call(bidRequests[i], ADUNIT_CODE, bid)); + bidRequests.forEach(bidRequest => callbacks.adapterDone.call(bidRequest)); + } + + function delay(ms = 0) { + return new Promise((resolve) => { + setTimeout(resolve, ms) + }); + } + + beforeEach(() => { + promiseSandbox.restore(); + 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(); + }); + + 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; + }); + }); + }); + }); + + 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('auctionOptions', function() { let bidRequests; let doneSpy; let clock; - let auction = { - getBidRequests: () => bidRequests, - getAuctionId: () => '1', - addBidReceived: () => true, - getTimeout: () => 1000 - } + let auction; let requiredBidder = BIDDER_CODE; let requiredBidder1 = BIDDER_CODE1; let secondaryBidder = 'doNotWaitForMe'; @@ -1352,7 +1476,15 @@ describe('auctionmanager.js', function () { 'auctionOptions': { secondaryBidders: [ secondaryBidder ] } - }) + }); + const start = Date.now(); + auction = { + getBidRequests: () => bidRequests, + getAuctionId: () => '1', + addBidReceived: () => true, + getTimeout: () => 1000, + getAuctionStart: () => start, + } }); afterEach(() => { diff --git a/test/spec/modules/currency_spec.js b/test/spec/modules/currency_spec.js index ccd205964a9..77215cea7f4 100644 --- a/test/spec/modules/currency_spec.js +++ b/test/spec/modules/currency_spec.js @@ -9,7 +9,8 @@ import { setConfig, addBidResponseHook, currencySupportEnabled, - currencyRates + currencyRates, + ready } from 'modules/currency.js'; var assert = require('chai').assert; @@ -24,6 +25,7 @@ describe('currency', function () { beforeEach(function () { fakeCurrencyFileServer = sinon.fakeServer.create(); + ready.reset(); }); afterEach(function () { @@ -286,7 +288,7 @@ describe('currency', function () { }); describe('currency.addBidResponseDecorator bidResponseQueue', function () { - it('not run until currency rates file is loaded', function () { + it('not run until currency rates file is loaded', function (done) { setConfig({}); fakeCurrencyFileServer.respondWith(JSON.stringify(getCurrencyRates())); @@ -296,14 +298,27 @@ describe('currency', function () { setConfig({ 'adServerCurrency': 'JPY' }); var marker = false; - addBidResponseHook(function() { + let promiseResolved = false; + addBidResponseHook(Object.assign(function() { marker = true; - }, 'elementId', bid); + }, { + bail: function (promise) { + promise.then(() => promiseResolved = true); + } + }), 'elementId', bid); expect(marker).to.equal(false); - fakeCurrencyFileServer.respond(); - expect(marker).to.equal(true); + setTimeout(() => { + expect(promiseResolved).to.be.false; + fakeCurrencyFileServer.respond(); + + setTimeout(() => { + expect(marker).to.equal(true); + expect(promiseResolved).to.be.true; + done(); + }); + }); }); }); diff --git a/test/spec/unit/pbjs_api_spec.js b/test/spec/unit/pbjs_api_spec.js index 2522887bb98..84e2d854b2c 100644 --- a/test/spec/unit/pbjs_api_spec.js +++ b/test/spec/unit/pbjs_api_spec.js @@ -16,6 +16,7 @@ import * as auctionModule from 'src/auction.js'; import { registerBidder } from 'src/adapters/bidderFactory.js'; import { _sendAdToCreative } from 'src/secureCreatives.js'; import find from 'core-js-pure/features/array/find.js'; +import {synchronizePromise} from '../../helpers/syncPromise.js'; var assert = require('chai').assert; var expect = require('chai').expect; @@ -190,13 +191,16 @@ window.apntag = { } describe('Unit: Prebid Module', function () { - let bidExpiryStub; + let bidExpiryStub, promiseSandbox; beforeEach(function () { + promiseSandbox = sinon.createSandbox(); + synchronizePromise(promiseSandbox); bidExpiryStub = sinon.stub(filters, 'isBidNotExpired').callsFake(() => true); configObj.setConfig({ useBidCache: true }); }); afterEach(function() { + promiseSandbox.restore(); $$PREBID_GLOBAL$$.adUnits = []; bidExpiryStub.restore(); configObj.setConfig({ useBidCache: false });