Re: Operations in invalid states: Exceptions don't make sense.

On 6/4/13 07:52, Adam Bergkvist wrote:
> On 2013-06-01 02:04, Justin Uberti wrote:
>> Was a decision reached here? It seems unfortunate to have to add
>> success/failure callbacks to several methods just to better handle being
>> called in the "closed" state.
>
> No, I don't think that would be nice.

Anything that can be queued needs to handle async failures, which means 
that all queueable actions need failure callbacks.

> The spec is a bit inconsistent at the moment but I think that 
> setLocal/RemoteDescription handles this fairly well. The state is 
> checked for closed before the real work is handed over to the WebRTC 
> backend. That makes these methods consistent with other methods (e.g. 
> addStream) that doesn't have success/failure callbacks; they don't do 
> anything on a closed PeerConnection.

The thing here is that we've identified situations in which things like 
addStream needs to be able to return errors in an asynchronous fashion 
also. Most of these are internal errors of one kind or another, but some 
of them can result from resource exhaustion.

I'm also not certain we have the list of queueable event correct yet. 
The ordering of, say, addStream versus createOffer is clearly important. 
The fact that one can be queued behind other operations while the other 
cannot will cause application developers more than a little surprise. 
The same goes for addIceCandidate.

In practice, by the time we have this fully implemented, I suspect we'll 
find reasons to queue most or all operations on RTCPeerConnection.

That said, I agree with the general *principle* that we don't want to 
add failure callbacks to methods for the exclusive purpose of reporting 
a closed condition. I just don't think we have that many places where we 
don't need failure callbacks for other reasons as well. Let's look at 
the current operations:

createOffer - has a failure callback

createAnswer - has a failure callback

setLocalDescription - has a failure callback

setRemoteDescription - has a failure callback

updateIce - We have not implemented this yet. I suspect, when we get 
around to doing so, we'll find that it needs to be async and allow error 
callbacks for reasons other than signling state.

addIceCandidate - Per conversation back in January, really needs a 
failure callback.

getLocalStreams - See below

getRemoteStreams - See below

getStreamById - See below

addStream - Needs a failure callback for resource exhaustion purposes

removeStream - As I argue above, since this can change the behavior of 
the operations we already know need to be queued, we'll eventually 
determine that it needs queueing also -- which means callbacks.

close - We've agreed that close() on a closed connection generates 
neither an error nor an exception

For getLocalStreams, getRemoteStreams, and getStreamById, I think we 
need to handle these the same way we treat attempts to read 
localDescription and remoteDescription on a closed connection. A naive 
reading of the spec right now indicates that reading localDescription on 
a closed connection returns whatever the localDescription was when it 
was closed (since there is no special handling described for reading 
these attributes on a closed connection). If this is what we want, the 
spec should make such behavior explicit. If we want something different 
(e.g., throwing an exception or returning a null), then we need to add 
text to the document to clarify such behavior.

/a

Received on Tuesday, 4 June 2013 15:53:52 UTC