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

Prebid core: fix exception in requestBids introduced by #9106 #9194

Merged
merged 4 commits into from
Nov 3, 2022
Merged

Prebid core: fix exception in requestBids introduced by #9106 #9194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf5ee30
Revert "Prebid core: return a promise from `requestBids` (#9106)"
dgirardi Nov 2, 2022
1a820a3
Revert "Revert "Prebid core: return a promise from `requestBids` (#91…
dgirardi Nov 2, 2022
58a35ae
Prebid core: fix exception in `requestBids` introduced by #9106
dgirardi Nov 2, 2022
4b9dada
Add comments
dgirardi Nov 3, 2022
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
162 changes: 83 additions & 79 deletions src/prebid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import {default as adapterManager, gdprDataHandler, getS2SBidderSet, uspDataHand
import CONSTANTS from './constants.json';
import * as events from './events.js';
import {newMetrics, useMetrics} from './utils/perfMetrics.js';
import {defer} from './utils/promise.js';

const $$PREBID_GLOBAL$$ = getGlobal();
const { triggerUserSyncs } = userSync;
Expand Down Expand Up @@ -622,31 +623,36 @@ $$PREBID_GLOBAL$$.removeAdUnit = function (adUnitCode) {
* @alias module:pbjs.requestBids
*/
$$PREBID_GLOBAL$$.requestBids = (function() {
const delegate = hook('sync', function ({ bidsBackHandler, timeout, adUnits, adUnitCodes, labels, auctionId, ttlBuffer, ortb2, metrics } = {}) {
const delegate = hook('async', function ({ bidsBackHandler, timeout, adUnits, adUnitCodes, labels, auctionId, ttlBuffer, ortb2, metrics, defer } = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I discovered the issue a little more comments would have helped what the intention of this delegate method was or what it is supposed to do.

events.emit(REQUEST_BIDS);
const cbTimeout = timeout || config.getConfig('bidderTimeout');
logInfo('Invoking $$PREBID_GLOBAL$$.requestBids', arguments);
const ortb2Fragments = {
global: mergeDeep({}, config.getAnyConfig('ortb2') || {}, ortb2 || {}),
bidder: Object.fromEntries(Object.entries(config.getBidderConfig()).map(([bidder, cfg]) => [bidder, cfg.ortb2]).filter(([_, ortb2]) => ortb2 != null))
}
return startAuction({bidsBackHandler, timeout: cbTimeout, adUnits, adUnitCodes, labels, auctionId, ttlBuffer, ortb2Fragments, metrics});
return startAuction({bidsBackHandler, timeout: cbTimeout, adUnits, adUnitCodes, labels, auctionId, ttlBuffer, ortb2Fragments, metrics, defer});
}, 'requestBids');

return wrapHook(delegate, function requestBids(req = {}) {
// if the request does not specify adUnits, clone the global adUnit array - before
// any hook has a chance to run.
// unlike the main body of `delegate`, this runs before any other hook has a chance to;
// it's also not restricted in its return value in the way `async` hooks are.

// if the request does not specify adUnits, clone the global adUnit array;
// otherwise, if the caller goes on to use addAdUnits/removeAdUnits, any asynchronous logic
// in any hook might see their effects.
req.metrics = newMetrics();
req.metrics.checkpoint('requestBids');
let adUnits = req.adUnits || $$PREBID_GLOBAL$$.adUnits;
req.adUnits = (isArray(adUnits) ? adUnits.slice() : [adUnits]);
return delegate.call(this, req);

req.metrics = newMetrics();
req.metrics.checkpoint('requestBids');
req.defer = defer({promiseFactory: (r) => new Promise(r)})
delegate.call(this, req);
return req.defer.promise;
Comment on lines +649 to +651
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a few comments why this is necessary would be helpful.

As someone only looking at bid adapters mostly the fun-hook framework and the custom promise implementation this code is non-obivous why it's there.

Copy link
Collaborator Author

@dgirardi dgirardi Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If you're asking about the reason for fun-hooks or the custom promise:

  • fun-hooks is basically a plugin system (to the author: sorry if I'm oversimplifying this). if we have 5 modules that may be loaded in a random combination and order, and all plug into the same code path, fun-hook solves the problem of orchestrating those. I can't answer why we are using that over something else.
  • the custom promise - A version of Promise that runs callbacks synchronously when it can - came about because of this discussion.

As for this code specifically, I tried clarifying it in the comments - it boils down to idiosyncrasies of fun-hooks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarification 🤗

});
})();

export const startAuction = hook('sync', function ({ bidsBackHandler, timeout: cbTimeout, adUnits, ttlBuffer, adUnitCodes, labels, auctionId, ortb2Fragments, metrics } = {}) {
export const startAuction = hook('async', function ({ bidsBackHandler, timeout: cbTimeout, adUnits, ttlBuffer, adUnitCodes, labels, auctionId, ortb2Fragments, metrics, defer } = {}) {
const s2sBidders = getS2SBidderSet(config.getConfig('s2sConfig') || []);
adUnits = useMetrics(metrics).measureTime('requestBids.validate', () => checkAdUnitSetup(adUnits));

Expand All @@ -658,85 +664,83 @@ export const startAuction = hook('sync', function ({ bidsBackHandler, timeout: c
adUnitCodes = adUnits && adUnits.map(unit => unit.code);
}

return new Promise((resolve) => {
function auctionDone(bids, timedOut, auctionId) {
if (typeof bidsBackHandler === 'function') {
try {
bidsBackHandler(bids, timedOut, auctionId);
} catch (e) {
logError('Error executing bidsBackHandler', null, e);
}
function auctionDone(bids, timedOut, auctionId) {
if (typeof bidsBackHandler === 'function') {
try {
bidsBackHandler(bids, timedOut, auctionId);
} catch (e) {
logError('Error executing bidsBackHandler', null, e);
}
resolve({bids, timedOut, auctionId});
}
defer.resolve({bids, timedOut, auctionId})
}

/*
* for a given adunit which supports a set of mediaTypes
* and a given bidder which supports a set of mediaTypes
* a bidder is eligible to participate on the adunit
* if it supports at least one of the mediaTypes on the adunit
*/
adUnits.forEach(adUnit => {
// get the adunit's mediaTypes, defaulting to banner if mediaTypes isn't present
const adUnitMediaTypes = Object.keys(adUnit.mediaTypes || { 'banner': 'banner' });

/*
* for a given adunit which supports a set of mediaTypes
* and a given bidder which supports a set of mediaTypes
* a bidder is eligible to participate on the adunit
* if it supports at least one of the mediaTypes on the adunit
*/
adUnits.forEach(adUnit => {
// get the adunit's mediaTypes, defaulting to banner if mediaTypes isn't present
const adUnitMediaTypes = Object.keys(adUnit.mediaTypes || { 'banner': 'banner' });

// get the bidder's mediaTypes
const allBidders = adUnit.bids.map(bid => bid.bidder);
const bidderRegistry = adapterManager.bidderRegistry;

const bidders = allBidders.filter(bidder => !s2sBidders.has(bidder));

const tid = adUnit.ortb2Imp?.ext?.tid || generateUUID();
adUnit.transactionId = tid;
if (ttlBuffer != null && !adUnit.hasOwnProperty('ttlBuffer')) {
adUnit.ttlBuffer = ttlBuffer;
// get the bidder's mediaTypes
const allBidders = adUnit.bids.map(bid => bid.bidder);
const bidderRegistry = adapterManager.bidderRegistry;

const bidders = allBidders.filter(bidder => !s2sBidders.has(bidder));

const tid = adUnit.ortb2Imp?.ext?.tid || generateUUID();
adUnit.transactionId = tid;
if (ttlBuffer != null && !adUnit.hasOwnProperty('ttlBuffer')) {
adUnit.ttlBuffer = ttlBuffer;
}
// Populate ortb2Imp.ext.tid with transactionId. Specifying a transaction ID per item in the ortb impression array, lets multiple transaction IDs be transmitted in a single bid request.
deepSetValue(adUnit, 'ortb2Imp.ext.tid', tid);

bidders.forEach(bidder => {
const adapter = bidderRegistry[bidder];
const spec = adapter && adapter.getSpec && adapter.getSpec();
// banner is default if not specified in spec
const bidderMediaTypes = (spec && spec.supportedMediaTypes) || ['banner'];

// check if the bidder's mediaTypes are not in the adUnit's mediaTypes
const bidderEligible = adUnitMediaTypes.some(type => includes(bidderMediaTypes, type));
if (!bidderEligible) {
// drop the bidder from the ad unit if it's not compatible
logWarn(unsupportedBidderMessage(adUnit, bidder));
adUnit.bids = adUnit.bids.filter(bid => bid.bidder !== bidder);
} else {
adunitCounter.incrementBidderRequestsCounter(adUnit.code, bidder);
}
// Populate ortb2Imp.ext.tid with transactionId. Specifying a transaction ID per item in the ortb impression array, lets multiple transaction IDs be transmitted in a single bid request.
deepSetValue(adUnit, 'ortb2Imp.ext.tid', tid);

bidders.forEach(bidder => {
const adapter = bidderRegistry[bidder];
const spec = adapter && adapter.getSpec && adapter.getSpec();
// banner is default if not specified in spec
const bidderMediaTypes = (spec && spec.supportedMediaTypes) || ['banner'];

// check if the bidder's mediaTypes are not in the adUnit's mediaTypes
const bidderEligible = adUnitMediaTypes.some(type => includes(bidderMediaTypes, type));
if (!bidderEligible) {
// drop the bidder from the ad unit if it's not compatible
logWarn(unsupportedBidderMessage(adUnit, bidder));
adUnit.bids = adUnit.bids.filter(bid => bid.bidder !== bidder);
} else {
adunitCounter.incrementBidderRequestsCounter(adUnit.code, bidder);
}
});
adunitCounter.incrementRequestsCounter(adUnit.code);
});
adunitCounter.incrementRequestsCounter(adUnit.code);
});

if (!adUnits || adUnits.length === 0) {
logMessage('No adUnits configured. No bids requested.');
auctionDone();
} else {
const auction = auctionManager.createAuction({
adUnits,
adUnitCodes,
callback: auctionDone,
cbTimeout,
labels,
auctionId,
ortb2Fragments,
metrics,
});

let adUnitsLen = adUnits.length;
if (adUnitsLen > 15) {
logInfo(`Current auction ${auction.getAuctionId()} contains ${adUnitsLen} adUnits.`, adUnits);
}
if (!adUnits || adUnits.length === 0) {
logMessage('No adUnits configured. No bids requested.');
auctionDone();
} else {
const auction = auctionManager.createAuction({
adUnits,
adUnitCodes,
callback: auctionDone,
cbTimeout,
labels,
auctionId,
ortb2Fragments,
metrics,
});

adUnitCodes.forEach(code => targeting.setLatestAuctionForAdUnit(code, auction.getAuctionId()));
auction.callBids();
let adUnitsLen = adUnits.length;
if (adUnitsLen > 15) {
logInfo(`Current auction ${auction.getAuctionId()} contains ${adUnitsLen} adUnits.`, adUnits);
}
});

adUnitCodes.forEach(code => targeting.setLatestAuctionForAdUnit(code, auction.getAuctionId()));
auction.callBids();
}
}, 'startAuction');

export function executeCallbacks(fn, reqBidsConfigObj) {
Expand Down
18 changes: 17 additions & 1 deletion test/spec/unit/pbjs_api_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,6 +1615,19 @@ describe('Unit: Prebid Module', function () {
});

describe('returns a promise that resolves', () => {
function delayHook(next, ...args) {
setTimeout(() => next(...args))
}

beforeEach(() => {
// make sure the return value works correctly when hooks give up priority
$$PREBID_GLOBAL$$.requestBids.before(delayHook)
});

afterEach(() => {
$$PREBID_GLOBAL$$.requestBids.getHooks({hook: delayHook}).remove();
});

Object.entries({
'immediately, without bidsBackHandler': (req) => $$PREBID_GLOBAL$$.requestBids(req),
'after bidsBackHandler': (() => {
Expand Down Expand Up @@ -1665,7 +1678,10 @@ describe('Unit: Prebid Module', function () {
sinon.assert.match(bids[bid.adUnitCode].bids[0], bid)
done();
});
completeAuction([bid]);
// `completeAuction` won't work until we're out of `delayHook`
// and the mocked auction has been set up;
// setTimeout here takes us after the setTimeout in `delayHook`
setTimeout(() => completeAuction([bid]));
Copy link
Collaborator

Choose a reason for hiding this comment

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

also a small comment here why wrapping in setTimeout is required here

})
})
})
Expand Down