Further work on promise handling: #655
Conversation
1. Convert the other fields (except resolveToConfig, which is special) to behave the way perBuyerSignals does, simplifying the language some and making them more web-y. 2. Make perBuyerCurrencies handle promises as well. 3. Hopefully fix a bug where we weren't actually waiting on promises for top-level config in a 2-level auction.
spec.bs
Outdated
| @@ -837,6 +838,7 @@ To <dfn>generate and score bids</dfn> given an [=auction config=] |auctionConfig | |||
| |auctionConfig|, |global|, and |settings|. | |||
| 1. If |compWinner| is failure, return failure. | |||
| 1. If |compWinner| is not null: | |||
| 1. If [=waiting until configuration input promises resolve=] given |auctionConfig| returns failure, return failure. | |||
There was a problem hiding this comment.
Doing this inside a parallel queue will mean that all subsequent steps get delayed. That's probably fine in this case since the wait has the same condition for all of them, and so subsequent ones won't wait anymore, but that means you can probably move it up in front of the overall loop so that future reviewers don't worry about this.
There was a problem hiding this comment.
I am unclear as to what you mean by subsequent here given the, well, parallelism.
What this is trying to achieve, at any rate, is this:
- Component auctions only need to wait for their own configuration to resolve to start bidding process.
- Top-level auction needs the whole hierarchy before starting its scoring, but its own scoring happens after a component makes a bid (before this change, it could still have some promises hanging out).
We do not want to delay Can COWL be useful for ths use case? #1 for promises in top-level config, which is what I think your suggestion would do?
Actually, mistake #1: the top-level needs all component auctions to have a complete configuration, not just the one who bid its considering, since it needs to pass an entire AuctionConfig to scoreAd.
Mistake #2: we do wait for configs for nested auctions even if they did not actually make a bid in top-level auction, in order to type-check them (though that can race with other reasons for auction to fail).
... Working on fixing these...
There was a problem hiding this comment.
...And hopefully done.
| @@ -722,6 +692,7 @@ To <dfn>validate and convert auction ad config</dfn> given an {{AuctionAdConfig} | |||
| 1. If |config|["{{AuctionAdConfig/resolveToConfig}}"] [=map/exists=]: | |||
| 1. Let |auctionConfig|'s [=auction config/resolve to config=] be | |||
| |config|["{{AuctionAdConfig/resolveToConfig}}"]. | |||
| 1. TODO: What should happen if this rejects? | |||
There was a problem hiding this comment.
You can either pretend it wasn't passed or reject the whole auction, right? It looks like other rejected promises bubble up to failing the whole auction, so this one probably should too. But no need to do that in this PR.
I'm actually surprised this needs to be async, since it affects the return type of runAdAuction(). Isn't the expected return type compiled into the code statically?
There was a problem hiding this comment.
typedef (USVString or FencedFrameConfig) UrnOrConfig;
Promise<UrnOrConfig?> runAdAuction(AuctionAdConfig config);
Yeah, impl-wise rejecting this also aborts the whole auction, which I guess could be done by setting this to failure and checking for that when accessing it, just like for others (unless we totally change the approach).
... This one promise is a bit different since it's needed much, much, later than others, hence the "input promise" terminology in my helpers
| @@ -836,6 +807,7 @@ To <dfn>generate and score bids</dfn> given an [=auction config=] |auctionConfig | |||
| 1. Let |compWinner| be the result of running [=generate and score bids=] with |component|, | |||
| |auctionConfig|, |global|, and |settings|. | |||
| 1. If |compWinner| is failure, return failure. | |||
| 1. If [=recursively wait until configuration input promises resolve=] given |auctionConfig| returns failure, return failure. | |||
There was a problem hiding this comment.
You should either remove the parallel queue, and write:
[=list/For each=] |component| in |auctionConfig|'s [=auction config/component auctions=],
run the following steps [=in parallel=]:
Or move this to before "If auctionConfig’s component auctions are not empty:".
There was a problem hiding this comment.
That would run into trouble with concurrently trying to update leadingBidInfo. It may be reasonable to split [=score and rank a bid=] into two parts, one functional (scoring), and one for updating leadingBidInfo ("ranking"), in which case all the scoring could be parallel, and then ranking be done on collected results...
... but at any rate, I would prefer if that wasn't part of this particular CL. As is, this line is a correctness fix...
There was a problem hiding this comment.
I'm happy to make this part of #662 instead of this PR.
There was a problem hiding this comment.
I would still prefer this line here --- it's imperfect, but it makes the spec run correct auctions correctly but slower-than-necessary rather than crash.
SHA: aa7c04c Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Preview | Diff