RE: [sysapps/raw socket api]: Status on outstanding issues and proposal to close a number of issues.

On Jul 3, 2013 7:32 AM, "Nilsson, Claes1" <Claes1.Nilsson@sonymobile.com>
wrote:
>
> Thanks again Jonas for very valuable input,
>
>
>
> See my replies inline below.
>
>
>
> BR
>
>   Claes
>
>
>
> From: Jonas Sicking [mailto:jonas@sicking.cc]
> Sent: den 3 juli 2013 07:59
>
> To: Nilsson, Claes1
> Cc: public-sysapps@w3.org
> Subject: Re: [sysapps/raw socket api]: Status on outstanding issues and
proposal to close a number of issues.
>
>
>
> On Thu, Jun 27, 2013 at 8:47 AM, Nilsson, Claes1 <
Claes1.Nilsson@sonymobile.com> wrote:
>>
>> Error handling to be revised,
https://github.com/sysapps/raw-sockets/issues/18:
>>
>> I have made a number of changes that address this issue. See
https://github.com/sysapps/raw-sockets/pull/23. Can I close the issue?
>
>
>
> I think the defined error handling in at least a few cases still need to
be improved. I looked through the error handling for TCP, but I expect that
some of these comments apply to UDP as well.
>
> When setting up a new connection, there should be machine readable errors
indicating is the failure was due to lack of network access, the port
already being taken, the receiving side refusing the connection, etc. Right
now the page just seems to receive a human readable string describing the
type of error which makes it hard for an application to do error handling.
>
> I suggest we use separate errors here rather than the generic
"NetworkError". Alternatively subclass DOMError and add another property
which contains an enum describing the exact type of error.
>
>
> I suspect the same applies for the case when an already established
connection is dropped.
>
> [Claes] I am a bit confused on how error handling should be specified in
new W3C specifications. For exceptions the
http://darobin.github.io/api-design-cookbook/ says that it
is strongly recommended that new specifications do not define new
exceptions, but rather reuse DOMException.

This is indeed very much not clear. What I think it intends to refer to is
that we should not mint new exception *classes*. DOMException is an
exception class, and can represent errors such as "NetworkError" and
"QuoteLimitError".

So minting need exception classes should be avoided, but minting new errors
for DOMException I don't think is a problem.

See the IndexedDB spec for an example.

> However, the cookbook says nothing about error handling based on
asynchronous events. My assumption so far has been that the general rule is
to use the DOMError interface

Correct

> but here we have the problem that the available values in the
DOM-specification for the “name” attribute don’t provide the granularity we
need. As the error names we need here are very specific to this API
specification it is no good idea to propose new error names to be added to
the DOM-specification. I have seen other recent W3C specifications that use
unique error interfaces instead of DOMError. Is this ok? Which approach is
best from a “W3C consistency point of view”.

The intended pattern is to subclass DOMError when you need to add more
granular information. So we can create something like

Interface SocketError : DOMError {
  readonly attribute DOMString type;
};

Where SocketError.name can be set to "NetworkError" or other errors as
needed.

Or some such. I would recommend doing something like this and then sending
an email to the Webapps list and ask for review.

> The constructor algorithm requires the implementation to synchronously
know if this application is already using the requested port/address combo,
and then throw an error. This is problematic if different windows of the
application are run in different processes since it requires blocking IPC.
Additionally it means that the error "address/port combo already in use" is
delivered in two different ways depending on if the combo is used by this
application or another application.
>
> [Claes] Yes, I have considered to remove the statement on exception
handling if the requested port/address combo is in use in the constructor
description’s 4th step and just rely on the asynchronous handling of
opening the socket as described further down. Would that address your
concern?

Yes.

> What does step 3 in the "send" algorithm mean by "data is too long to
pass atomically through the underlying protocol"? The underlying protocol
here is TCP, does that mean that attempting to send() anything that doesn't
fit in a single TCP packet should cause the connection to be dropped? I
would have expected the data to be divided into several TCP packets
automatically.
>
> [Claes] Ok, this is not applicable for TCP. I will remove “or because the
data is too long to pass atomically through the underlying protocol”.

Its also not needed for UDP, right?

> In the same step, I think it's a problem that we change the readystate to
"closed" synchronously. That means that if the following code:
>
> socket.send(arraybuffer1);
> socket.send(arraybuffer2);
>
> is unsafe. Since the first call to .send() could cause the readystate to
synchronously change, the second call could throw an exception. Instead I
think we should queue a task which does all of the substeps of step 3
(which removes the need for any of the substeps to queue tasks).
>
> [Claes] I see the problem but I am not sure that I understand your
proposed resolution.

Right now the text says:

3. If an error occurs, run the following substeps.
  1. Set readyState to "closed"
  ...
  4. Queue a task that fires an error event
  5. Queue a task that fires a close event

Change that to

3. If an error occurs set the internal IgnoreAllSendCalls flag and queue a
task that runs the following substeps.
  1. Set readyState to "closed"
  ...
  4. Fire an error event
  5. Fire a close event

You should also change the send algorithm such that it acts as a no-op
whenever the IgnoreAllSendCalls flag is set.

/ Jonas