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

The order in which Promises are returned, rejected, or resolved is ambiguous. #535

Closed
jernoble opened this issue May 19, 2015 · 26 comments
Closed
Assignees
Labels
Needs Edits Decision has been made, the issue can be fixed. https://speced.github.io/spec-maintenance/about/
Milestone

Comments

@jernoble
Copy link
Member

The spec mentions that Promises returned from suspend(), resume(), and close() should be resolved or rejected, but not whether that should occur before or after the Promise is returned. (Exception: the spec does say "immediately" in the case of suspend()ing an already suspended context, but even that could be construed ambiguously.

Chrome rejects/resolves these promises after returning them; WebKit is investigating rejecting/resolving them before returning. (Ref: WebKit bug 145164).

The ambiguity could be removed by rewording these sections to read, e.g., in resume(): "If the context is already running, return a resolved Promise.", "If the context is already closed, return a rejected Promise." if the intention is to resolve/reject before returning, or: "If the context is already running, queue a task to resolve the Promise." if not.

@jernoble
Copy link
Member Author

See also Issue #501.

@youennf
Copy link

youennf commented May 19, 2015

I may be wrong, but looking at Chrome source code, it seems that the returned promises are already rejected/resolved when being returned.

I agree though that rewriting the description of suspend resume and close would be nice.
It might be good also to specify which value is used to resolve.
It is probably undefined but it could be understood as null as well.

I like the current description of the streams API, for instance https://streams.spec.whatwg.org/#release-readable-stream-reader

@padenot
Copy link
Member

padenot commented May 19, 2015

I read the spec as "returning already rejected/resolved Promises", that's what is currently implemented in Gecko. I think that is indeed what Chrome does as well. @rtoy, is that correct ?

I think it makes sense, and I can take care of writing the new text if everybody agrees.

@rtoy
Copy link
Member

rtoy commented May 19, 2015

The very latest Chrome returns promises for suspend() that are resolved/rejected. (This is because Chrome currently has no way of determining when the audio HW has actually stopped.) For resume(), the returned promise isn't yet resolved/rejected because we're waiting for the audio HW to restart. When the audio is restarted, the promises are then resolved.

@jernoble
Copy link
Member Author

@rtoy If you look at the webkit.org bug I referenced above, @youennf has written a test that shows that (EDIT: in Chrome) "returned resolved" promises from the Web Audio APIs have their then callbacks called after bare promises created later in JavaScript. So this may well be just an implementation bug rather than a difference in interpretation. However, I still think that "If ... return a rejected promise" is clearer than "The promise is rejected if ...", with respect to timing.

@rtoy
Copy link
Member

rtoy commented May 20, 2015

@jernoble Thanks for the test. I ran it with ToT chromium, and it passes now. We recently fixed a couple of issues where we weren't quite handling suspend and resume correctly. (Resume didn't always resume, and the promise for suspend was not handled correctly so that you couldn't resume when handling the promise.)

@mark-buer
Copy link
Contributor

Time to play devil's advocate...

What benefit is there, from an API consumers perspective, for the resolution ordering to be exactly specified?

A couple of counter arguments to changing the spec:

  • leaving the spec intentionally ambiguous wrt resolution of the promise leaves room for flexibility in implementation design,
  • consumers shouldn't write code (or be encouraged to write code) where this difference between pre or post resolution of the promise has any effect

The last point is especially true for cases where the promise is provided by a third-party.

@jernoble
Copy link
Member Author

@mark-buer Counter-devil's-argument: If that is a valid purpose of a spec, then we should add language such that implementers must randomly change the resolution order of Promises so as to keep page authors from being able to depend on any particular resolution order.

If we leave it ambiguous, the end result is that some (important) website will pick a behavior which is compatible in Browser A, and incompatible in Browser B, and Browser B's implementers will be forced to change their behavior to match Browser A, and what ends up being de-facto specced is whatever behavior is implemented by the largest browser, not what behavior is best or makes the most sense.

The Web APIs specification defines a strict ordering for asynchronous tasks, with very precisely defined cardinals of ambiguity: http://www.w3.org/TR/html5/webappapis.html#event-loops. I don't see why we should add ambiguity here.

(Edit: corrected a wide variety of typos.)

@mark-buer
Copy link
Contributor

@jernoble Re-reading this issue I realise that many points are being raised, and my previous comment cherry picked only a small fraction of the total content. It makes me feel ashamed at how poorly I understood the issue and at my failure to articulate my thoughts, I apologise.

My "devil's advocation" was a reaction to the notion that an API consumer might race one promise (JS created) against another (returned from the API) to determine whether (for example) the API has already acquired access to the audio hardware or not. It's difficult to imagine why this would be useful unless someone was intentionally trying to break things. I stand behind my prior comment stating that nothing should be done about this case, it doesn't seem a useful thing to fix by itself.

I agree with the broader notion that the promises returned by suspend, resume and close must have a well specified resolution ordering with respect to each other and with respect to themselves. It is especially relevant for the case where multiple suspend and/or resumes are invoked in close succession. Details about the transitionary states: "suspending", "resuming" and "closing", seems to be lacking despite these states potentially existing for non-zero periods of time. Additionally, the exact timing for updating the externally visible audioContextState (state) property is not well specified. Also, what should happen when close() is invoked when the audio context is already in the closed state? The return of a rejected promise?

I wonder if all state transition requests, and resolution of their resulting promises, should be sequentially-ordered with respect to resolution of prior returned promises.
This would essentially form a promise chain.

A runnable thought-experiment:

var transitionable = Promise.resolve();
var state = 'running';

function transitionToState(destinationState, transitioner) {
    // Await completion of the previous state transition before attempting the new state transition
    transitionable = transitionable.then(function () {
        if (state === 'closed') {
            return Promise.reject();
        }
        if (state === destinationState) {
            return;
        }
        return transitioner().then(function() {
            state = destinationState;
        });
    });
    return transitionable;
}

function resume() {
    return transitionToState('running', function() {
        return new Promise(function(resolve, reject) {
            // Start acquiring hardware access... invoke resolve when hardware access is obtained
            setTimeout(function() {
                resolve();
            }, 500);
        });
    });
}

function suspend() {
    return transitionToState('suspended', function() {
        return new Promise(function(resolve, reject) {
            // Start releasing hardware access... invoke resolve when hardware access is released
            setTimeout(function() {
                resolve();
            }, 500);
        });
    });
}

function close() {
    return transitionToState('closed', function() {
        return new Promise(function(resolve, reject) {
            // Shutdown the audio context... invoke resolve when shutdown complete
            setTimeout(function() {
                resolve();
            }, 500);
        });
    });
}

edit: minor typo fix, & cosmetic code alterations

@jernoble
Copy link
Member Author

@mark-buer

I wonder if all state transition requests, and resolution of their resulting promises, should be sequentially-ordered with respect to resolution of prior returned promises.

So, as far as I understand it, this is the point of the Web API spec's Event loops section: that all asynchronous APIs should be defined in terms of Tasks and Task Queues, which would then have a strictly defined order of task execution. The "cardinals of ambiguity" I mentioned earlier in that spec is that UAs can decide to service some task queues more frequently than others (e.g., Browser A may chose to service mouse and touch events more frequently than networking tasks, and vice versa for Browser B). As a result, there's already an explicit lack of a guarantee about global task ordering.

With this framework in mind, the question you raise can be stated as: should all Promises share a task queue for resolution, or should Web Audio-created Promises have a separate task queue from JavaScript-created Promises. Since all event dispatch shares a task queue, I think it makes sense for all Promise resolution to share one as well.

@mdjp mdjp modified the milestone: Uncommitted Jun 18, 2015
@joeberkovitz joeberkovitz added the Needs Discussion The issue needs more discussion before it can be fixed. label Jun 18, 2015
@joeberkovitz
Copy link
Contributor

@cwilso to discuss with TAG

@joeberkovitz joeberkovitz modified the milestones: Web Audio V1, Uncommitted Jun 18, 2015
@padenot
Copy link
Member

padenot commented Oct 6, 2015

I've been reworking this, now that we have more of the underlying concepts defined: http://padenot.github.io/web-audio-api/#widl-AudioContext-suspend-Promise-void, for example.

Promise resolution vs. Tasks and event dispatching is well defined in the Web Platform, and with this new text, it should hopefully be clear what to implement.

@joeberkovitz
Copy link
Contributor

@padenot do you want to take this over from @cwilso and create a PR, if this is now straightforward?

@padenot
Copy link
Member

padenot commented Oct 8, 2015

It is already done I believe (barring any mistake I might have done), see the link to my draft above.

@jernoble, can you have a quick look at the above text and see if it's good enough ? It seemed logical for me to have the same task queue for all the content (web audio and other things), I don't see any reason why web audio would be different here. Promise are resolved at microtask checkpoints and that's well defined as well.

@jernoble
Copy link
Member Author

jernoble commented Oct 8, 2015

@padenot Sure thing.

Each AudioContext has a single control message queue, that is a list of control message, that are a operations running on the control thread.

I feel like this is sentence is missing a word in the last clause. "...that are created by operations running on the control thread."?

This looks a lot like a task queue, as defined in the HTML specification. Task queues are owned not by a document, but are shared across all related browsing contexts, and tasks enqueued into them are executed in the order in which they were enqueued across those browsing contexts. I.e., if two browsing contexts A & B enqueued tasks in this order: ABABAB, they should be executed in that order and not, for instance, in the order AAABBB.

I mention this because that task execution ordering has been a source of subtle browser bugs in the past.

There is probably less of a risk of browser bugs since communication between AudioWorkers must occur through messages sent to the main thread, but I think it bears mentioning.

Apart from that (possible, minor) issue, the changes look good to me.

@padenot
Copy link
Member

padenot commented Oct 8, 2015

My prose is a bit unclear, it should read:

Each AudioContext has a single control message queue, that is a list of control message. Control
messages execute operations running on the control thread.

It looks like a task queue but it is not the HTML task queue. The ordering is defined, main thread posts tasks to the AudioContext control message queue in a specific order, and it executes on the control thread in the same order. I you post messages back to the main thread during a control thread operation, it gets linearized in the main thread task queue.

@padenot padenot assigned padenot and unassigned cwilso Oct 8, 2015
@padenot
Copy link
Member

padenot commented Oct 8, 2015

Jer said during the call that we need to think on whether this works fine when multiple audiocontexts are sharing the same rendering thread, and I agree it's something we need to consider (as at least Gecko does it, possibly others).

@joeberkovitz
Copy link
Contributor

I'm having trouble thinking of a way in which using multiple AudioContexts change anything here, providing they share the same rendering thread. There are still two threads: a control thread, and a rendering thread. The same ordering constraints would apply, regardless of which AudioContext is involved.

If the AudioContexts don't share the same rendering thread, that's where things could get weird in terms of ordering. In that case different threads could process messages separately, in an order different from the order in which they were posted by the main thread. But the ordering constraints would still apply in this case within the scope of messages to/from any given AudioContext's rendering thread. Which seems sufficient to me, provided we document it.

@jernoble
Copy link
Member Author

jernoble commented Oct 8, 2015

@joeberkovitz

I'm having trouble thinking of a way in which using multiple AudioContexts change anything here, providing they share the same rendering thread. There are still two threads: a control thread, and a rendering thread. The same ordering constraints would apply, regardless of which AudioContext is involved.

The distinction would be, whether if two AudioContexts, A & B, both share an rendering thread, then the spec as written would have control queue messages which were enqueued in order ABABAB be executed in order AAABBB. This may not be a problem, since (in theory) the rendering side of each AudioContext should be incapable of affecting the rendering side of any other AudioContext. But since many of these control messages result in promises resolved or events fired back on the control thread, this ordering is observable to script.

If the AudioContexts don't share the same rendering thread, that's where things could get weird in terms of ordering.

That's true, and UAs which use multiple rendering threads will have that behavior be observable to script as well. (Though this point may well be moot if no UAs are planning on doing so.)

@rtoy
Copy link
Member

rtoy commented Oct 8, 2015

I'm pretty sure chrome creates a different audio thread for each audio context.

@joeberkovitz
Copy link
Contributor

@jernoble Given what I'm hearing, I believe that as long as ordering is preserved in the communication between the control thread and any given AudioContext's rendering thread then the system works in a sane and well-defined way. I do not think we should impose ordering across messages that interact with different AudioContexts; this should be speced as indeterminate (just as it would be if one were interacting with different WebWorkers). Paul's processing model may need to be refined so that this is clear (although my reading of it is that ordering is already scoped to a particular AudioContext).

@jernoble
Copy link
Member Author

jernoble commented Oct 9, 2015

@rtoy

I'm pretty sure chrome creates a different audio thread for each audio context.

Even for multiple audio contexts within the same browsing context? (Say, two AudioContext's within the same page.)

@jernoble
Copy link
Member Author

jernoble commented Oct 9, 2015

@joeberkovitz

Given what I'm hearing, I believe that as long as ordering is preserved in the communication between the control thread and any given AudioContext's rendering thread then the system works in a sane and well-defined way.

That's true, it's just different than the ordering defined by HTML on tasks in event queues, whose executions are ordered across all their "related similar-order browsing contexts". See: http://www.w3.org/html/wg/drafts/html/master/webappapis.html#event-loops

On the other hand, HTML also specifies the ordering of tasks within a Worker context http://www.w3.org/TR/workers/#the-event-loop. This may be relevant even if we redefine AudioWorker to be some "not-quite-a-worker" object.

I do not think we should impose ordering across messages that interact with different AudioContexts; this should be speced as indeterminate (just as it would be if one were interacting with different WebWorkers). Paul's processing model may need to be refined so that this is clear (although my reading of it is that ordering is already scoped to a particular AudioContext).

I am fine with having this ordering be explicitly indeterminate.

@joeberkovitz
Copy link
Contributor

@jernoble We seem to agree on the outcome. FWIW my reading of http://www.w3.org/TR/workers/#the-event-loop is that it specifies exactly the kind of indeterminate ordering across different Workers' event loops that we will have with AudioWorker.

@joeberkovitz
Copy link
Contributor

We can close this once #647 is merged, including commit 6d93bda

@joeberkovitz joeberkovitz added Needs Edits Decision has been made, the issue can be fixed. https://speced.github.io/spec-maintenance/about/ and removed Needs Discussion The issue needs more discussion before it can be fixed. labels Oct 27, 2015
@padenot
Copy link
Member

padenot commented Sep 19, 2016

#647 has been closed, closing this. Synchronous sections have all been annotated.

@padenot padenot closed this as completed Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Edits Decision has been made, the issue can be fixed. https://speced.github.io/spec-maintenance/about/
Projects
None yet
Development

No branches or pull requests

8 participants