[spec] Fix semantics around Abort Signal for runAdAuction() #1266
Conversation
| @@ -1679,6 +1685,7 @@ and a [=real time reporting contributions map=] |realTimeContributionsMap|: | |||
| "top-level-auction", null, |topLevelOrigin|, and |realTimeContributionsMap|. | |||
| 1. Decrement |pendingComponentAuctions| by 1. | |||
| 1. Wait until |pendingComponentAuctions| is 0. | |||
There was a problem hiding this comment.
So github doesn't seem to let me comment on the line I want, so I will do it here:
I think you need to change wait until configuration input promises resolve stuff to look for aborts, otherwise you might not terminate when you should.
Though... network fetches worry me too, like what happens if the auction is aborted and also they don't reply?
There was a problem hiding this comment.
That's a very good point. I have modified so that wait until configuration input promises resolve fails when an abort occurs. I didn't check that Chrome has that behavior, but if not Chrome is probably wrong.
As for network fetches, I think that may be a problem, but I think it's an existing issue that affects auctions that are not aborted. I think that should wait until another CL.
There was a problem hiding this comment.
That's a very good point. I have modified so that
wait until configuration input promises resolvefails when an abort occurs. I didn't check that Chrome has that behavior, but if not Chrome is probably wrong.
Chrome renderer will mojo to the browser the moment the abort algorithm is run, and the browser will pick it up in the event loop and abort the auction. So there is no worry about things being stuck --- but there is an inherent race there since the browser may be wrapping up the auction execution concurrently to the renderer calling abort, so the auction may still succeed.
As for network fetches, I think that may be a problem, but I think it's an existing issue that affects auctions that are not aborted. I think that should wait until another CL.
So my expectation would be that it would be fine for a non-aborted auction to wait for fetches, and that aborted ones shouldn't wait for fetches.... except in Chrome we also set a 30s timeout on fetches.
There was a problem hiding this comment.
Yeah, I tried to capture the possibility that the auction succeeds during an abort with this spec change. There's still some existing weirdness there I haven't handled in this CL (returning failure from a parallel queue doesn't really make sense -- probably need to fix things up and "abort these steps"), but I would prefer to fix that later.
Technically we don't need to spec things without side effects and I don't think waiting for fetches for an aborted auction would be visibly different from not waiting. I guess you could argue that the server could tell if a client disconnects before the body is sent, but does that matter. Do we need to spec whether or not to abort the fetches?
I think the timeout is something we probably should spec though.
There was a problem hiding this comment.
Hmm. Fetches are side-effects, though whether they happen or not is racey anyway? I guess we can probably just call it a quality-of-implementation type of deal --- a good implementation will cancel as much work as possible, but as the page shouldn't be able to collude with the target of fetches anyway, it shouldn't matter for correctness...
| @@ -1679,6 +1685,7 @@ and a [=real time reporting contributions map=] |realTimeContributionsMap|: | |||
| "top-level-auction", null, |topLevelOrigin|, and |realTimeContributionsMap|. | |||
| 1. Decrement |pendingComponentAuctions| by 1. | |||
| 1. Wait until |pendingComponentAuctions| is 0. | |||
There was a problem hiding this comment.
Hmm. Fetches are side-effects, though whether they happen or not is racey anyway? I guess we can probably just call it a quality-of-implementation type of deal --- a good implementation will cancel as much work as possible, but as the page shouldn't be able to collude with the target of fetches anyway, it shouldn't matter for correctness...
Co-authored-by: qingxinwu <[email protected]>
SHA: 9a4a270 Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Preview | Diff