Queue tasks from in parallel

[npm1]

Since DiscoverFromExternalSource is run in parallel, this PR does some changes:

1. Queue tasks when we need to fetch or create a new Credential object.
2. Add 'on' methods which receive the fetch result and continue the execution of the FedCM algorithm with the results from the fetch.

Relevant issue: #261

---

Preview | Diff

[npm1]

CC @bvandersloot-mozilla @annevk in case you're interested in reviewing.

[npm1]

I added a bunch of 'on' functions to receive the fetch results. @johannhof, could you take a look?

[npm1]

Ping @samuelgoto can you take a look please?

[npm1]

@domfarolino any chance you could take a look? I believe this PR is not too complicated and improves Fetch integration but Sam didn't feel comfortable taking the first look. Perhaps you can lend us your Fetch expertise :)

[domfarolino]

Here a few comments from skimming through this this evening, but I'll give it a more thorough look later.

[npm1]

Thanks for the review so far!

[domfarolino]

One thing I'd recommend is prefacing each algorithm with its return type, so we can more-easily track that all of the return values are appropriately used. I think I spotted some cases where algorithms were returning things like "false" and "an empty list" to nobody that was doing anything with those values, so it might be good to structure the algorithms as I mentioned, and do an audit.

The common way to do this is:

"To foo given a bar and baz, run the following steps. They return a qux."

[npm1]

One thing I'd recommend is prefacing each algorithm with its return type, so we can more-easily track that all of the return values are appropriately used. I think I spotted some cases where algorithms were returning things like "false" and "an empty list" to nobody that was doing anything with those values, so it might be good to structure the algorithms as I mentioned, and do an audit.

The common way to do this is:

"To foo given a bar and baz, run the following steps. They return a qux."

Thanks, added return type to the algorithms which had one.

[npm1]

Ready for another review! Added asserts and removed some unneeded task queueing.

[domfarolino]

This generally LGTM. I think it's probably fine to clean the hand-wavy "Wait" part up in a follow-up, provided we clarify what we're actually doing with the thing we're waiting for.

[npm1]

@samuelgoto @domfarolino lemme know if there are any objections to merging this! I want to get this merged to work on other stuff in the spec (this touches everything so easier to have it merged before)

[samuelgoto]

Will look at this first thing today! Thanks for setting this up!

…

On Mon, Jan 9, 2023, 8:01 AM npm1 ***@***.***> wrote: @samuelgoto <https://github.com/samuelgoto> @domfarolino <https://github.com/domfarolino> lemme know if there are any objections to merging this! I want to get this merged to work on other stuff in the spec (this touches everything so easier to have it merged before) — Reply to this email directly, view it on GitHub <#378 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFJL2SLNFEXA4MQ4UIFBBTWRQY67ANCNFSM6AAAAAASIKQQZQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[samuelgoto]

This PR is looking excellent: massive progress in making the spec a lot more precise in terms of Fetch and being concrete about the algorithms. I made a few suggestions to try to remove further the chaining and use "async/await" more to make it easier to read, but otherwise this is LGTM.

Thanks for putting this together, looking forward to merging this!

[domfarolino]

I haven't had time to look deeply at the changes to the chaining and "async/await" as @samuelgoto as put it, can you summarize it briefly?

[npm1]

I haven't had time to look deeply at the changes to the chaining and "async/await" as @samuelgoto as put it, can you summarize it briefly?

In the latest change, I used the following style. In an algorithm (eg. fetch X):

1. Steps in the DOM manipulation task source.
2. Let |returnVar| be null
3. Fetch and in the fetch callback set |returnVar|.
4. In parallel, wait for |returnVar| to become non-null, and return it.

That is kinda the closest I could get to the await/async syntax that Sam wanted to simplify the methods / avoid a long chaining of callbacks.

[samuelgoto]

LGTM

@domfarolino can you take a last look and approve too?

Once @domfarolino LGTMs and this last round of feedback is addressed (I think there is a slightly cleaner way to deal with the async/awaits), I'm planning to squash and merge.

[domfarolino]

In the latest change, I used the following style. In an algorithm (eg. fetch X):

2. Let |returnVar| be null
3. Fetch and in the fetch callback set |returnVar|.
4. In parallel, wait for |returnVar| to become non-null, and return it.

That is kinda the closest I could get to the await/async syntax that Sam wanted to simplify the methods / avoid a long chaining of callbacks.

On first scan, this looks nice but I think it has some issues. First, you'd need to be more explicit about posting tasks. An algorithm that has its control flow broken up by "Waiting in parallel" would need to explicitly come back to the original-calling thread to return a value. The common trend I see for this elsewhere is:

1. Do some main-thread stuff
2. Wait in-parallel for some main-thread state to change, then post a task back to the main thread to run the following steps
3. (Back on main thread), do something with the value `foo` that we now know has been changed to what we want

That would normally work, depending on what you do with foo. The problem with your algorithms, it seems, is that they need to return the value foo that you waited for in a non-blocking way. But I don't think you can really return from an algorithm once its control flow has been interrupted by waiting "in-parallel" (as to not block the main thread). By the time we come back to the main thread in step 3, the original caller of the algorithm has already totally finished invoking it, so I don't think we can return a concrete value after that. That's why async/await works in JS: async algorithms don't actually return concrete values, they all return promises so that the caller can only retrieve the value asynchronously after the algorithm was invoked, but I don't think we have a spec prose equivalent.

Do your algorithms have to return values, or can they just modify state? That is, can you do the following? :

4. In parallel, wait for `returnVar` to become non-null, and do something with it (but not return it)

[npm1]

Hey @samuelgoto @domfarolino the latest PR has changes that hopefully satisfy both of you. Now most stuff is run in parallel and we only go into a task when necessary, thus making it possible to wait for the results of fetches while still allowing the algorithms to return the awaited values. PTAL and let me know what you think!

[samuelgoto]

The last commit makes changes that I think meet the readability bar that I was trying to maintain. If this meets the fetch precision bar, I think we should merge this.

Dominic, WDYT?

[samuelgoto]

I'm still waiting some confirmation from farolino that this works, but here is another round of feedback while we wait for his approval.

Again, I think this is ready to be merged as far as I'm concerned, and these can be resolved in a separate PR.

[domfarolino]

I think |check| doesn't exist at this point yet because it was defined for the first time in the fetch response handler.

[npm1]

Removed |check| and now just waiting for 'both fetch responses to be completed'. Let me know if that works?

[domfarolino]

Before I do a full scan, here's one example of a red flag I'm seeing kind of frequently: https://github.com/fedidcg/FedCM/pull/378/files#diff-40cc3a1ba233cc3ca7b6d5873260da9676f6ae20bb897b62f7871c80d0bda4e9R1284. Basically we're throwing a DOMException from in parallel at least 4 times in that algorithm, and I think elsewhere as well. But DOMExceptions are platform objects and must be created/thrown with a surrounding agent / global, so I don't think what's happening now is technically valid. This is why I mentioned on chat the other day when we discussed introducing "queue fetch":

As long as everything besides those steps can run in parallel and doesn't need to be on the main thread
Which I wasn't under the impression of earlier, but if that's the case that would be great

It seems like there are more things besides Fetch that currently expect to run on the main thread, which is a little worrying. I'm not sure what the full implementation flow is in Chromium so I don't know what to recommend for these algorithms, but it seems like spec-wise they all used to run on the main thread with the exception of "waiting" for fetches to complete, but the "waiting" got complicated and callback-ey, so you switched it so the algorithms ran entirely "in parallel" with the exception of kicking off main-thread tasks for URL parsing and Fetching, but this causes two issues:

1. There are other things that have to run on the main thread that are currently not
2. While the way we're kicking off tasks for URL parsing and Fetching is technically correct, it kind of hints to me that there is a design issue. I think it's uncommon to make fetches where the URL is parsed asynchronously with respect to the Fetching, and this can have observable consequences too (i.e., you're allowing the <base> URL of the document to change in between parsing and fetching, whereas I'm guessing that is not possible in the implementation?).

I think (1) needs to be fixed regardless, but (2) might just be a symptom of the unsafe-no-cors Fetch mode not being ironed out yet. For example, as per whatwg/fetch#1533 (comment) we're going to want to Fetch these requests from some sort of detached context (I think in-parallel), whereas this PR sticks with today's precedent of only fetching from the main thread, which is best for now, but is in part why we have this weird asynchronous gap between parsing and Fetching and maybe more things. I think (2) can probably be overlooked for now, though (1) shouldn't be. But I don't know the best way to fix (1) without reverting the algorithms to run on the main thread and wait in parallel, but I am happy to hear other ideas @npm1 since you probably have the spec paged into your head more than me right now.

[npm1]

Before I do a full scan, here's one example of a red flag I'm seeing kind of frequently: https://github.com/fedidcg/FedCM/pull/378/files#diff-40cc3a1ba233cc3ca7b6d5873260da9676f6ae20bb897b62f7871c80d0bda4e9R1284. Basically we're throwing a DOMException from in parallel at least 4 times in that algorithm, and I think elsewhere as well. But DOMExceptions are platform objects and must be created/thrown with a surrounding agent / global, so I don't think what's happening now is technically valid.

Hmm yea, perhaps we can a) change those cases to 'return null', and then at the main method just queue a task to throw if the result was null b) add a helper so that any throw becomes 'queue a task that throws', and change all throws to also return too (this might be tricky because the parent will not know, so I think a) is better).

It seems like there are more things besides Fetch that currently expect to run on the main thread, which is a little worrying. I'm not sure what the full implementation flow is in Chromium

The algorithm in Chromium is implemented in the browser process. Unfortunately there is no such equivalent in the spec? It seems that the closest thing is 'in parallel' but we are forced to go back to main for things that supposedly cannot be handled in parallel, but which we can do in the browser process.

2. While the way we're kicking off tasks for URL parsing and Fetching is technically correct, it kind of hints to me that there is a design issue. I think it's uncommon to make fetches where the URL is parsed asynchronously with respect to the Fetching, and this can have observable consequences too (i.e., you're allowing the <base> URL of the document to change in between parsing and fetching, whereas I'm guessing that is not possible in the implementation?).

Yea that's true although the fetches are not relative to the document.

I think (1) needs to be fixed regardless

Is there anything else you see besides the exceptions? We could do what I suggested if that's the only thing, but I may have missed other examples.

[npm1]

By the way the exception thing is a problem coming from the caller as well as this already assumes you can throw exceptions from in parallel: https://w3c.github.io/webappsec-credential-management/#ref-for-dom-credential-discoverfromexternalsource-slot%E2%91%A0:~:text=Set%20result%20to%20the%20result%20of%20executing%20result%E2%80%99s

But thinking about it, why is that not allowed? They are thrown and caught from in parallel (and I'm assuming we're always carrying around the |globalObject| anyways).

[npm1]

Answering to myself: at least DOMExceptions are Objects so they cannot be created from in parallel. I will return failure except at the end, and there 'queue a task to throw'.

[npm1]

I think I fixed exception handling for the most part, let me know what you think!

[domfarolino]

This seems to break up the list and reset the counter at 1 in the rendered HTML. I'm not sure what the solution for that is to be honest, but maybe you have to resort to manual numbering?

[npm1]

I vehemently oppose manual numbering :) It seems issues/notes require an extra indentation compared with numbered items (that fixed it, here and in a couple other places).

[npm1]

ptal

[npm1]

I vehemently oppose manual numbering :) It seems issues/notes require an extra indentation compared with numbered items (that fixed it, here and in a couple other places).

[domfarolino]

OK it looks like the exception problem is fixed now with the exception (pun intended) of one case I think? Otherwise, I think your "failure" propagation works nicely.

[domfarolino]

I think the "network error" here should be a failure like you now have elsewhere, since this is all in parallel.

[npm1]

Agree but DiscoverFromExternalSource actually accepts exceptions being thrown at it somehow. I filed w3c/webappsec-credential-management#211 since I'm not sure that's acceptable (plus need the globalObject). I suspect it is done that way so that error messages can be set by whoever implements the DiscoverFromExternalSource. So yes this method is still throwing exceptions in two places, but I propose waiting for the linked issue to be resolved before modifying.

[domfarolino]

That seems fine, while slightly unfortunate 👍