Fix for early auction close with video + done cb cleanup #3024
Conversation
src/auction.js
Outdated
| let doneCalled = 0; | ||
| return function() { | ||
| doneCalled++; | ||
| if (doneCalled === bidderCount) { |
There was a problem hiding this comment.
are we concerned that some bidders will call multiple done callbacks causing the auction to close early?
| } | ||
| } | ||
|
|
||
| function adapterDone() { |
There was a problem hiding this comment.
what do we need adapterDone for?
|
Honestly was pretty confused reading the code. Let's walk through this a bit. |
mkendall07
changed the title
There was a problem hiding this comment.
Hi @jaiminpanchal27 I did some testing with multiple video ad units and mix of instream/outstream and it seems like the auction is completing successfully after the bids were cached.
See some additional points to consider below.
src/auction.js
Outdated
| } | ||
|
|
||
| function adapterDone() { | ||
| // events.emit(CONSTANTS.EVENTS.BIDDER_DONE, bidderRequest); |
There was a problem hiding this comment.
Can remove commented code (if we truly do not need to emit this event here).
| let bidRequest = getBidderRequest(bidRequests, bid.bidderCode, adUnitCode); | ||
| let bidResponse = getPreparedBidForAuction({adUnitCode, bid, bidRequest, auctionId}); | ||
|
|
||
| if (bidResponse.mediaType === 'video') { |
There was a problem hiding this comment.
Should add logic (here?) to clarify only for instream video adUnits, since outstream video adUnits shouldn't need to be cached.
src/auction.js
Outdated
| @@ -316,7 +356,7 @@ function addBidToAuction(auctionInstance, bidResponse) { | |||
| } | |||
|
|
|||
| // Video bids may fail if the cache is down, or there's trouble on the network. | |||
| function tryAddVideoBid(auctionInstance, bidResponse, bidRequest) { | |||
| function tryAddVideoBid(auctionInstance, bidResponse, bidRequest, afterBidAdded) { | |||
There was a problem hiding this comment.
Could remove bidRequest from arguments here since it doesn't appear to be used now with these new changes.
# Conflicts: # test/spec/auctionmanager_spec.js
* done cb cleanup * rename some vars * added unit tests, updates after changes requested * add deprecated notice to function
* done cb cleanup * rename some vars * added unit tests, updates after changes requested * add deprecated notice to function
Type of change
Description of change
Cleaning up done callback which closes the auction. Simplifying the design.