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

when closing, all outstanding actions are cancelled and their callbacks are fired with a "cancelled" error #150

Closed
dontcallmedom opened this issue Dec 15, 2014 · 15 comments
Assignees

Comments

@dontcallmedom
Copy link
Member

Originally reported on W3C Bugzilla ISSUE-25835 Tue, 20 May 2014 20:15:58 GMT
Reported by Cullen Jennings
Assigned to

@dontcallmedom
Copy link
Member Author

Original comment by Martin Thomson on W3C Bugzilla. Tue, 04 Nov 2014 23:14:27 GMT

#22

@alvestrand
Copy link
Contributor

The "queue a task" rewrite does not seem to have addressed this issue.

@dontcallmedom
Copy link
Member Author

The "queue a task" doesn't address it, but it seems to be that most of the individual methods do:

  • createAnswer and createOffer have:

    If this RTCPeerConnection object is closed before the SDP generation process completes, the USER agent MUST suppress the result and not resolve or reject the returned promise.

  • setLocalDescription and setRemoteDescription have:

    If connection's signaling state is closed, then abort these steps.

in the algorithm to reject a promise

addIceCandidate doesn't describe the detailed algorithm of its asynchronous process (see #336) where one would expect that rule to be used.

(I'm certainly not opposed of moving this in "queue a task" if it's possible in a way that works for all the methods)

@alvestrand
Copy link
Contributor

The original bug says we agreed at some point that outstanding tasks should be cancelled (that is, promises rejected) at close.
The current text Dom's citing seems to indicate the opposite (that they hang around forever).
@jan-ivar what's considered kosher when using promises - abort them, or leave them unfulfilled?

@adam-be
Copy link
Member

adam-be commented Oct 16, 2015

From what I've seen (in e.g. EventSource and WebSocket), it's quite common to abort a task initiated by the User Agent if the object in question has entered a closed state.

What makes our case a bit different is that it's sometimes directly related to an API request (and not a message from the network).

Perhaps a ping to the public-script-coord list is in place since this should be handled consistently between promise based APIs.

@adam-be
Copy link
Member

adam-be commented Oct 16, 2015

The Promise Writing Guide has a section [1] that talks about good and bad situations for rejection.

"Any situation where something is internally broken while attempting an asynchronous operation: for example if the developer passes in invalid data, or the environment is in an invalid state for this operation."

The above quote is a good case for rejection. The last part is somewhat our case. Ideally, the "closed state when done case" should be explicitly mentioned in that section.

[1] https://www.w3.org/2001/tag/doc/promises-guide#rejections-should-be-exceptional

@jan-ivar
Copy link
Member

The script-coord discussion culminated in https://lists.w3.org/Archives/Public/public-script-coord/2015OctDec/0037.html :

Based on these answers, it seems like leaving it pending forever is probably the right way to go.

This seems reasonable to me.

@adam-be
Copy link
Member

adam-be commented Nov 3, 2015

Thanks for picking up that thread @jan-ivar. The resolution seems reasonable to me too.

@alvestrand
Copy link
Contributor

The conclusion at TPAC was that the arguments for leaving the promises dangling were not compelling enough that we should change the specified behavior - the promises should be rejected on close.

@adam-be
Copy link
Member

adam-be commented Nov 4, 2015

My initial thought about that is that we will have error handers firing for cases that aren't really error cases (but rather cancel cases as mentioned in the email thread). I'll take a look at the minutes when they are available.

@alvestrand
Copy link
Contributor

The thought at the meeting was that these are cases where success is guaranteed to never happen. If we signal failure, the application can choose to disregard this particular failure; if we don't, we foreclose the option of acting on it.

@adam-be
Copy link
Member

adam-be commented Nov 4, 2015

I understand the reasoning. The argument against it may be that it's perhaps not so JavaScript-API-ish. We make commonly used code paths more complicated to cater for an edge case.

I just want us to have a good reason for ignoring the expert advice we asked for and got, but this may not be the right place to do it.

@jan-ivar
Copy link
Member

The conclusion at TPAC was that the arguments for leaving the promises dangling were not compelling enough that we should change the specified behavior - the promises should be rejected on close.

@alvestrand I recall the consensus at TPAC was to "leave as-is". The spec says to not reject on close.

createOffer and createAnswer:
"If this RTCPeerConnection object is closed before the SDP generation process completes, the USER agent MUST suppress the result and not resolve or reject the returned promise."

setLocalDescription and setRemoteDescription:
"2. If connection's signaling state is closed, then abort these steps."

@alvestrand
Copy link
Contributor

We'll leave it as is (no action on close).

@mhofman
Copy link

mhofman commented Jan 13, 2016

Sorry I missed this, but looking at the minutes, the discussion was phrased around rejecting the promise on close, and most being in favor of it: https://www.w3.org/2015/10/29-webrtc-minutes.html

This seem to be different from what is recorded here.

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

No branches or pull requests

6 participants