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

On Thu, May 23, 2013 at 2:13 AM, Adam Bergkvist <adam.bergkvist@ericsson.com
> wrote:

> On 2013-05-22 09:31, Adam Roach wrote:
>
>> Currently, the WebRTC spec has some implication that a PeerConnection's
>> signalingState should be checked at the time an operation (e.g.,
>> setRemoteDescription) is enqueued, and an that an exception should be
>> thrown if the state is not appropriate for the indicated operation /at
>> that time//,/ regardless of how the already-enqueued operations may
>>
>> change that state.
>>
>> This is covered in section 4.6.1 (Error Handling / General Principles):
>> " An exception /MUST/ be thrown... [if a] function call was made when
>>
>> the RTCPeerConnection is in an invalid state, or a state in which that
>> particular function is not allowed to be executed."
>>
>> This interacts badly with the conceptual model that we currently have
>> around operations, which is that scripts are allowed to make several
>> calls on the PeerConnection without first waiting for the prior one to
>> complete (with the result being that the PeerConnection queues such
>> operations until the previous one completes).
>>
>> Roughly speaking, this means that the PC state can be okay at the time
>> an operation is enqueued, but invalid by the time the operation is
>> executed. Conversely, the PC state may be invalid at the time an
>> operation is attempted, but be changed to an invalid state by the time
>> the operation is to be run.
>>
>> The first set of circumstances requires us to allow async error
>> callbacks for invalid states.
>>
>> The second set of circumstances means that synchronous checks will,
>> under some circumstances, throw exceptions when it is inappropriate to
>> do so.
>>
>> In other words, I think this points to an absolute requirement to check
>> states async and an absolute prohibition on checking them synchronously.
>>
>> The specific change I'm requesting is:
>>
>>   * Remove the second bullet from section 4.6.1
>>   * Change step 1 of "addStream" in section 4.3.2.2 to end with
>>
>>     "...abort these steps, and call the addStream error callback with an
>>     RTCError of  INVALID_STATE"
>>   * Remove step 1 from "close" in section 4.3.2.2
>>
>>   * Change step 2 of "addStream" in section 4.3.2.2 to end with
>>     "...abort these steps, and call the removeStream error callback with
>>     an RTCError of  INVALID_STATE"
>>   * Change the first bullet of "setLocalDescription" in section 4.3.2.2
>>
>>     to end with "...abort these steps, and call the setLocalDescription
>>     error callback with an RTCError of INVALID_STATE"
>>   * Change step 1 of section 5.1.2 to end with "...call the
>>
>>     createDataChannel error callback with an RTCError of INVALID_STATE."
>>   * Remove step 2 from section 5.2.3 (for this reason and for the reason
>>
>>     that readyState no longer exists)
>>   * Change step 1 of section 7.2.1 to end with "...call the getStats
>>
>>     error callback with an RTCError of INVALID_STATE."
>>   * Change step 1 of section 8.2.2, setIdentityProvider to end with
>>
>>     "...abort these steps, and call the setIdentityProvider error
>>     callback with an RTCError of  INVALID_STATE"
>>
>
>
> Thanks for bringing this up; there's some text that hasn't been properly
> updated WRT the operation queuing.
>
> Not all functions you mention above are candidates to be queued. The spec
> lists createOffer, setLocalDescription, createAnswer and
> setRemoteDescription [1]. It may be that this list should be updated.
>
> For the calls that may be queued, we have the option to split them up into
> a section that first runs on the main loop and checks if the PeerConnection
> isn't closed, and then adds a task to the queue (that also may need to
> check the state when executed). The first check would be valid since the
> PeerConnection can't recover from the closed state. This would be more
> consistent with non-queueable operations and would avoid queuing operations
> on a closed PeerConnection.
>

I don't see the advantage of this, it just seems like it's another place to
go
wrong. [0]

You can still detect closed in the main loop and then queue a task to
fire the error cb. This isn't hard and gives you a consistent interface.

-Ekr

[0] Note that we definitely see people calling .close() twice and having
it throw an exception is a recipe for problems.