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

Run to completion violation #12

Closed
domenic opened this issue May 9, 2019 · 15 comments
Closed

Run to completion violation #12

domenic opened this issue May 9, 2019 · 15 comments

Comments

@domenic
Copy link

domenic commented May 9, 2019

This API appears to violate https://w3ctag.github.io/design-principles/#js-rtc. It appears to do so fairly fundamentally, as part of its very purpose. In particular, it betrays the presence of input handling code that runs separately from the JavaScript thread, and that thread communicates its state in a way that is not the usual queue-a-task manner.

I'm not sure what to do here, but I think it's a pretty big architectural issue worth highlighting.

Also note that apart from the guidelines, this may violate some ECMAScript spec requirements; I'm not sure, as I think that would either be in the agents part of the spec (too complicated for me) or the jobs part of the spec (broken). /cc @littledan as he has been looking in to some event loop run-to-completion semantics. Also /cc @annevk, since he has been working on some related stuff around "spin the event loop", which per the way its spec is written violates run to completion. (But, from what we can tell, does so in a way that isn't observable via author code.)

I suppose now that the shared memory cat is out of the bag using SharedArrayBuffer, run to completion isn't what it used to be, and maybe this is just a second way to observe changes in other threads. In that case we should probably have the W3C TAG update their design principles.

/cc @slightlyoff @cwilso who were involved in the great WebAudio run-to-completion violation debate of years gone by.

@tdresser
Copy link
Collaborator

tdresser commented May 9, 2019

Thanks, great point.

"any data observed within the function will stay constant as long as that function is active."
Isn't this violated by performance.now() for example?

If we don't implement this, I anticipate once off thread input is supported, folks will build this themselves this via off thread input and SAB. I suspect the cat is, in fact, out of the bag on this one.

@domenic
Copy link
Author

domenic commented May 9, 2019

Yeah, how things like performance.now() or Date.now() fit with run to completion is an interesting question as well. I feel like I had a satisfactory answer once upon a time, but no longer remember it. I could indeed see an argument that this is similar to those.

@annevk
Copy link

annevk commented May 10, 2019

FWIW, I think it's quite bad to add new high-level APIs that violate this. That will make things much harder to reason about.

@ojanvafai
Copy link
Member

This doesn't seem like what the TAG text is actually referencing, although it is imprecise enough that I could be interpreted in different ways. Regardless we should probably get the text to be more precisely written. Specifically, this sentence is could use refining: "any data observed within the function will stay constant as long as that function is active."

In my head, the key constraint of run to completion is about avoiding races in JS state. Just having isInputPending return different values when called within a single function call doesn't seem to me like it violates run to completion.

An example of where run to completion is critical:
window.a = 5;
function foo() {
let a1 = window.a;
isInputPending();
let a2 = window.a;
// a1 has to equal a2
}

@HolgerJeromin
Copy link

The API timeRemaining() from the Background Tasks API seems to be similar. Or am I missing something?

@tdresser
Copy link
Collaborator

tdresser commented Jul 9, 2019

Another example: the complete attribute on img elements.

From the spec: "The value of complete can thus change while a script is executing."

@domenic
Copy link
Author

domenic commented Jul 9, 2019

That's not really true; see whatwg/html#1055

@annevk
Copy link

annevk commented Jul 10, 2019

Two things about recentish comments:

  • Time is special and where we expose time in this manner it's about a difference between some stored time and the current time, and the latter is already available otherwise.
  • Precedent violating the guideline doesn't mean it's a good idea. There's a feature in the DOM on which you can build a polyfill of sorts for weak references, but that does not mean we'd endorse any new API using that pattern. There's a higher bar for new APIs.

@tdresser
Copy link
Collaborator

Thanks Anne, I don't think examples are central to this discussion, though I do find them interesting.

Ojan's comment here does a better job of getting to the core of the issue.

@annevk
Copy link

annevk commented Jul 10, 2019

I think the key problem with the API is that the return value of isInputPending() can change at any point during the execution of a task in a non-deterministic manner.

@ojanvafai
Copy link
Member

@annevk it would help me have a more concrete debate about the trade-offs if you could give examples of specific cases where you are worried non-determinism from this API will cause unexpected or problematic behavior for web authors. For example, I gave sample code above of a case that I think is important not to break (although, last I checked, Gecko does violate even that in some edge cases).

Historical precedent may matter here if web authors don't seem to currently have problems with those APIs. That provides some evidence that that definition of run-to-completion might be more strict than is necessary.

@szager-chromium
Copy link

@ojanvafai Just food for thought, but isFramePending has very much the same issue.

@annevk
Copy link

annevk commented Aug 21, 2019

E.g., isInputPending(x) !== isInputPending(x).

I'm not sure complete has the same problem btw. At least reading that definition it seems like it can change if you manipulate attributes, but that's fairly normal behavior.

@dbaron
Copy link

dbaron commented Sep 10, 2019

For what it's worth (as I've mentioned in w3ctag/design-reviews#415 (comment)) I tend to think that the run-to-completion principle may be the sort of principle where accidental/hidden violations are more problematic than obvious ones. In other words, some random DOM API that violates run-to-completion because some boolean gets flipped from a different thread seems worse than things like Date.now() or IsInputPending() that are clearly asking questions that somebody would expect (we hope!) to change while code is running.

(This is similar to the line of thought that made me more OK with the GC observability aspects of WeakRef, i.e., that people who are explicitly using WeakRef know they're dealing with GC-dependent stuff, whereas if some random DOM property intermittently becomes null that's more likely to produce a result that developers didn't expect.)

That said, in both cases it's still possible for the violations of these principles to lead to bugs in sites, but it seems more defensible in this sort of case.

@n8schloss
Copy link
Collaborator

Closing based on @dbaron's comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants