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.

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

[Spec] Handle B&A server response promise #1285

Merged
merged 9 commits into from
Sep 30, 2024
Merged

Conversation

qingxinwu
Copy link
Collaborator

@qingxinwu qingxinwu commented Sep 25, 2024

@qingxinwu qingxinwu added the spec Relates to the spec label Sep 25, 2024
spec.bs Outdated
specified directly without a Promise.

* To parse the value |result|:
1. Set |auctionConfig|'s [=auction config/server response=] to
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check if this struct's field can be uint8array, or needs another Infra data structure, like byte sequence. If so, need to do convert javascript to infra value on it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's okay to leave it as a Uint8Array, but we probably need to copy or transfer (https://webidl.spec.whatwg.org/#arraybuffer-transfer) the underlying storage. UInt8Array is really just a shallow pointer to a buffer. We're going to be accessing it [=in parallel=] and we want to specify that later changes to the provided array in the main context won't affect the version we are using in the parallel context.

Copy link
Collaborator Author

@qingxinwu qingxinwu Sep 25, 2024

Choose a reason for hiding this comment

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

how about this https://webidl.spec.whatwg.org/#dfn-get-buffer-source-copy? I saw it used in fetch API (step 1.3 of https://fetch.spec.whatwg.org/#incrementally-read-loop). (oh, it notes "Implementations are strongly encouraged to use an implementation strategy that avoids this copy where possible").
Does it match our intention?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I would prefer a transfer here over a copy. With a transfer the value in the array is undefined after a call, but allows implementations to optimize. We perform a copy on Chrome because of IPC, but there's no reason to require a copy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

got it. Done.

@qingxinwu
Copy link
Collaborator Author

@brusshamilton PTAL. Thanks
@morlovich Since you wrote the other promise handling part, PTAL as well. Thanks!

spec.bs Outdated
specified directly without a Promise.

* To parse the value |result|:
1. Set |auctionConfig|'s [=auction config/server response=] to
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's okay to leave it as a Uint8Array, but we probably need to copy or transfer (https://webidl.spec.whatwg.org/#arraybuffer-transfer) the underlying storage. UInt8Array is really just a shallow pointer to a buffer. We're going to be accessing it [=in parallel=] and we want to specify that later changes to the provided array in the main context won't affect the version we are using in the parallel context.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@morlovich morlovich left a comment

Choose a reason for hiding this comment

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

LGTM

spec.bs Outdated
1. If |compWinnerInfo| is failure, abort these steps.
1. If [=recursively wait until configuration input promises resolve=] given |auctionConfig| returns
failure, abort these steps.
1. If |compWinnerInfo| is failure, or [=recursively wait until configuration input promises resolve=]
Copy link
Collaborator Author

@qingxinwu qingxinwu Sep 26, 2024

Choose a reason for hiding this comment

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

@morlovich After reading it more, I think we need to decrement |pendingComponentAuctions| in these cases, otherwise the step below that waits for |pendingComponentAuctions| to be 0 will keep waiting? WDYT

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. I think we may abort less then the implementation (just a component and not the whole thing) when this happens, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm yes, but if we check [=recursively wait until configuration input promises resolve=] first before "|compWinnerInfo| is failure" here, any component promise failure will fail all component auctions (not top level though, but top level would have no winner from component auctions)?
I think we've talked about this that the implementation of a component promise failure failing the whole auction is not perfect. Is there a plan to change that? I guess no, if that's the case, we'd be better to make the spec match the impl.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but the server response promise can fail in a subcomponent, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems component auction server response promise only fail a single component auction, not the whole auction (if I read https://crsrc.org/c/content/browser/interest_group/interest_group_auction.cc;drc=f522344e45882da4c7f7cb1b3a0a7bd747d654bb;l=5760 correctly, although not fully sure).

Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is messy. Failing high-level parsing on it fails just the component auction, but failing type conversion to uint8array or promise rejecting aborts the entire thing:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/ad_auction/navigator_auction.cc;drc=f522344e45882da4c7f7cb1b3a0a7bd747d654bb;l=3245

Copy link
Collaborator Author

@qingxinwu qingxinwu Sep 27, 2024

Choose a reason for hiding this comment

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

ah interesting. I think it would be better to make it consistent for this single promise? If so, failing the component auction only? @brusshamilton FYI as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it probably should just fail the component auction.

Copy link
Collaborator Author

@qingxinwu qingxinwu Sep 27, 2024

Choose a reason for hiding this comment

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

ok, (1) I changed it to check [=recursively wait until configuration input promises resolve=] first so that any (non server response) promise failure could fail the whole auction which meets our implementation (the impl's behaviour is not ideal but it's not easy to change and there's no plan to change it yet).
(2) For server response promise, I kept the behaviour of only fail the component auction no matter what was the failure (promise reject, resovled promise parse failure, etc,). It's different from our impl since promise reject fails the whole auction, but we do have a plan to change the impl and it feels doable. We can change the spec later if the plan changes.

spec.bs Outdated Show resolved Hide resolved
@JensenPaul JensenPaul merged commit f112a2a into WICG:main Sep 30, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Sep 30, 2024
SHA: f112a2a
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@qingxinwu qingxinwu deleted the promise branch September 30, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants