Core & currency module: fix bug where bid responses are collected after auction ends (prebid#10558)

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

* hook fn does not need to be a hook

Author: dgirardi

modules/currency.js

@@ -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
@@ -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) {
@@ -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';
@@ -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) {
@@ -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) {

src/auction.js

@@ -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');
@@ -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--;
@@ -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;
@@ -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));
     }
   }
 }

test/spec/auctionmanager_spec.js

@@ -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';
@@ -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', () => {

test/spec/modules/currency_spec.js

@@ -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';
@@ -32,7 +32,6 @@ describe('currency', function () {
 
   beforeEach(function () {
     fakeCurrencyFileServer = server;
-    ready.reset();
   });
 
   afterEach(function () {
@@ -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();
         });
       });