Add a way of aborting an auction - #342
Conversation
| ... | ||
| ] | ||
| ], | ||
| 'abortSignal': ..., |
There was a problem hiding this comment.
Should this field be called "signal" as per https://dom.spec.whatwg.org/#abortcontroller-api-integration?
There was a problem hiding this comment.
I thought that would be too confusing given things like browserSignals, etc, but I wasn't aware this was documented like this. Hmm.
Edit: looks like I also need to check if it's already rejected and wire the abort reason through.
There was a problem hiding this comment.
Yeah. 'signal' will be quite confusing in this context (even with Fetch API)...
There was a problem hiding this comment.
Sadly had to change to signal, will try to get the spec changed, but it doesn't look very encouraging.
|
|
||
| Optionally, `sellerTimeout` can be specified to restrict the runtime (in milliseconds) of the seller's `scoreAd()` script, and `perBuyerTimeouts` can be specified to restrict the runtime (in milliseconds) of particular buyer's `generateBid()` scripts. If no value is specified for the seller or a particular buyer, a default timeout of 50 ms will be selected. Any timeout higher than 500 ms will be clamped to 500 ms. A key of `'*'` in `perBuyerTimeouts` is used to change the default of unspecified buyers. | ||
|
|
||
| Optionally, the `abortSignal` field can be set to an [AbortSignal](https://dom.spec.whatwg.org/#interface-AbortSignal) object to permit aborting the execution of the auction. When the `abort()` method on the associated `AbortController` is called, an attempt to interrupt the auction will be made. Since the auction executes in parallel to the page, it's possible for this call to happen after the auction actually completed but before this has been noticed by the caller of `runAdAuction`. In that case, the cancellation attempt is ignored. If the cancellation is successful, the promise returned from `runAdAuction` will resolve to null. |
There was a problem hiding this comment.
I believe this functionality may help address an issue brought up in #318. That said, do we need to define the behavior around what happens with reporting (reportWin and reportResult) in the event when an auction is cancelled more explicitly? For example, if the cancellation is successful, would that signal a strong guarantee that reporting from the auction did not yet have a chance to happen?
There was a problem hiding this comment.
My understanding is that the reporting is done in the final step of runAdAuction() after the actual auction is completed. If the cancellation attempt is sent during the reporting period, it will be ignored. This is to guarantee no partial/conflict reporting being sent. Is this correct?
There was a problem hiding this comment.
Mostly. It indeed either it finishes, or reportWin/reportResult don't run.
I think some of the pending aggregation reports may still run, though. Perhaps they shouldn't?
Also some of the statistics on number of bids, etc., will get updated. That may be trickier...
There was a problem hiding this comment.
So I got some pushback on the possibility of abort failing on the CL that implements this. In the alternative, the promise will always (except if it already resolved?) be rejected, and all the work thrown out (this direction is conditional on the change to delay reporting till after the auction completes). What do you folks think of that?
(https://chromium-review.googlesource.com/c/chromium/src/+/3805648 if you folks want to look in, though the conversation is kinda scattered).
There was a problem hiding this comment.
Will this result in a race condition that is the promise gets rejected but the report is still sent? Imagine the cancellation request comes in during the reporting period.
There was a problem hiding this comment.
So current status is that if it's aborted successfully/promise is rejected no side effects happen --- no reporting, no updates of bid counts, no private aggregation requests, etc. If abort is too late and promise resolves, then it's a successful auction, with reporting and all.
There was a problem hiding this comment.
Thanks for the update Maks! Can we clarify those in the pull request?
There was a problem hiding this comment.
We are looking into using this feature. One thing I am not sure if it is implemented consistently is that when the auction is cancelled with AbortSignal, the promise should be rejected with error set to AbortSignal.reason which can be TimeoutError (if AbortSignal.timeout() is used) or an AbortError. In this way, we are able to differentiate a real error vs. timeout.
[1] https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout#return_value
There was a problem hiding this comment.
By looking at the code change, https://chromium-review.googlesource.com/c/chromium/src/+/3805648/21/third_party/blink/renderer/modules/ad_auction/navigator_auction.cc#1627
I believe it is implemented in this way. Just want to confirm.
There was a problem hiding this comment.
It does appear to work fine (at least from looking at the serialization in .catch()); there is also some test coverage of passing of reason via AbortController.signal:
|
|
||
| Optionally, `sellerTimeout` can be specified to restrict the runtime (in milliseconds) of the seller's `scoreAd()` script, and `perBuyerTimeouts` can be specified to restrict the runtime (in milliseconds) of particular buyer's `generateBid()` scripts. If no value is specified for the seller or a particular buyer, a default timeout of 50 ms will be selected. Any timeout higher than 500 ms will be clamped to 500 ms. A key of `'*'` in `perBuyerTimeouts` is used to change the default of unspecified buyers. | ||
|
|
||
| Optionally, the `abortSignal` field can be set to an [AbortSignal](https://dom.spec.whatwg.org/#interface-AbortSignal) object to permit aborting the execution of the auction. When the `abort()` method on the associated `AbortController` is called, an attempt to interrupt the auction will be made. Since the auction executes in parallel to the page, it's possible for this call to happen after the auction actually completed (perhaps unsuccessfully) but before this has been noticed by the caller of `runAdAuction`. In that case, the cancellation attempt is ignored. If the cancellation is successful, the promise is rejected. |
There was a problem hiding this comment.
This we need more details here - like how this interacts with setPriority() (Has that even been specced out yet?)
There was a problem hiding this comment.
... Why would it interact with setPriority?
There was a problem hiding this comment.
Oh, you mean in terms of updating the priority, not in terms of ordering? Yeah, good chance that doesn't match what I just wrote in the last push, may need to look at fixing that.
There was a problem hiding this comment.
Yes. Currently we update the priority immediately (even if the auction never completes for any reason).
7928bf4 to
04e4981
Compare
|
What version will this feature be available? |
|
The commit is in 107.0.5299.0 canary/dev. |
Match the AbortController spec a bit better Spec sync to impl Document the setPriority thing Comment on late abort
1493bc9 to
1abf5b6
Compare
SHA: 016264d Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
(Implementation is in review as of now)