sysapps / tcp-udp-sockets

Raw sockets API
86 stars 25 forks source link

Error handling to be revised #18

Open ClaesNilsson opened 11 years ago

ClaesNilsson commented 11 years ago

Generally the following rules apply:

Go through the specification and assure that this approach is fulfilled.

Specifcally for the UDP and TCPServerSocket constructors the following change should be done:

ClaesNilsson commented 11 years ago

Bakground to error handling modifications in Pull Request #34:

The background to theses updates on error handling is a mail dialogue between me and Jonas: http://lists.w3.org/Archives/Public/public-sysapps/2013Jul/0008.html http://lists.w3.org/Archives/Public/public-sysapps/2013Jul/0017.html http://lists.w3.org/Archives/Public/public-sysapps/2013Jul/0020.html

So, the basic problem with error handling is that with the standard DOMError interface the granularity of machine readable error names are too limited. An example is when the attempt to create a TCP socket fails. The reason can for example be "no network contact"," peer does not respond" and "local address/port pair is already in use". The available DOM error names does not make it possible to separate these error reasons and the only sensible error name for all these situations is "NetworkError".

In the previous version of the specification I tried to solve this problem by distinguishing between these error reasons with the DOMError message attribute. However, the message attribute contains an implementation dependent message to be displayed to the user. Using this attribute does not make machine readable handling of errors possible, i.e. the application can not be coded to handle these dirrent error reasons in different manners.

The http://darobin.github.io/api-design-cookbook/ does not give much guidence here. So after discussion with Jonas I took the approach of subclassing DOMError and added the "type" attribute that provides a more detailed error definition than the "name" attribute.

This problem with error handling is probably applicable for more W3C specifications and it would be good if the authors of the DOM-specification stated a recommended approach on how to define a more granular error handling.

ClaesNilsson commented 11 years ago

The approach I have taken in the Raw Socket API is a subclass of DOMError called SocketError that adds an attribute "type" that details the error in addition to the error names defined for DOMError. WDYT? Should we contact the WebApps WG to hear their option on this approach? If so, do you (or anyone else) know if www-dom@w3.org is the correct address for a question on this?

marcoscaceres commented 11 years ago

On Sunday, August 4, 2013, Claes Nilsson wrote:

The approach I have taken in the Raw Socket API is a subclass of DOMError called SocketError that adds an attribute "type" that details the error in addition to the error names defined for DOMError. WDYT? Should we contact the WebApps WG to hear their option on this approach? If so, do you (or anyone else) know if www-dom@w3.org<javascript:_e({}, 'cvml', 'www-dom@w3.org');>is the correct address for a question on this?

Yep, that's the right place to ask.

— Reply to this email directly or view it on GitHubhttps://github.com/sysapps/raw-sockets/issues/18#issuecomment-22075697 .

Marcos Caceres http://datadriven.com.au

ClaesNilsson commented 11 years ago

Reply from Anne van Kesteren:

"DOMError is going away. We only need DOMException. Allen (editor of ECMAScript) did suggest we do something like what you suggest. Have DOMException.prototype.subname which gives a more detailed name and ideally have DOMException.name be "DOMException" but we can probably no longer do that. Still need to work out the details here unfortunately as apparently last time we thought we figured error handling out we didn't :/"

So, I guess that I should keep the current solution in the Raw Socket API until the DOMException solution has been defined in the DOM specification.

asutherland commented 11 years ago

I'm not sure if this is the perfect issue for this, but in mozTCPSocket in Gecko for the Firefox OS e-mail app, we tried to add a number of specific error types and names related to network and security problems. The plan was to do what is proposed here, have a more generic type attribute and then some more specific error code information. Here is the webidl for what was desired but not implemented: https://bug867872.bugzilla.mozilla.org/attachment.cgi?id=744436

The implementing bug that added our error codes was https://bugzilla.mozilla.org/show_bug.cgi?id=861196 and resulted in the logic found here: https://github.com/mozilla/mozilla-central/blob/ba7e562edcd537b00aba393e065d964103f17348/dom/network/src/TCPSocket.js#L607

The specific error codes in use were the result of taking existing NSS error codes and weeding out the ones that Brian Smith indicated were going away imminently. This shouldn't be viewed as a specific proposal.

In order to avoid adding a ton of UI strings late in the release process which would be mean to localizers, we didn't map all of the error codes to distinct strings. But in general our e-mail app would like to be able at the very least to generate machine error codes for QA/support purposes even if we use a smaller subset of strings to explain that there is a security problem.

In general our error goal is to report these 3 categories: a security problem the user might be able to do something about (check the system clock), something that looks a lot like an attack against the user and therefore maybe they should try a different network, or something that just looks like a badly configured server. If we end up supporting adding certificate exceptions, we might end up wanting more information.

ClaesNilsson commented 11 years ago

Thanks a lot for this information Andrew. I am in the process of executing a set of updates to the specification. Then I will go through the error situations again and your input will be very helpful here. So I might come back with questions.

nickdesaulniers commented 10 years ago

In example 3, I found it ambiguous that the assignment of the onerror member of a TCPSocket instance would itself be wrapped in a try ... catch block. In general, it leaves the developer more flexibility to return them the error object and let them decide if they want to throw it, rather than just always throw. Of course, developers are not always keen on checking return variables though.

ClaesNilsson commented 10 years ago

The idea was to:

So for example an exception is thrown if the remoteAddress argument to the constructor is not a valid IPv4/6 address (a bug in the application code). On the other hand, if the attempt to establish a new TCP connection with the peer fails the error event is fired.

However, Marcos rewrote Examples 1 and 2 to not let the application catch the exeptions. This might be a better style and my plan is to rewrite examples 3 and 4 as well.

WDYT?

ClaesNilsson commented 10 years ago

When using promises rejection reasons should always be instances of the ECMAScript Error type such as DOMException or the built in ECMAScript error types. See https://github.com/w3ctag/promises-guide#rejection-reasons-should-be-errors.

In the TCP and UDP Socket API specification we need a better granularity of machine readable errors and therefore the SocketError interface (http://raw-sockets.sysapps.org/#interface-socketerror) was added. This interface extends DOMError with a machine readble error type providing a better granularity of error reporting. However, the SocketError interface can not be used by promises so I have temporily just stated a DOMException for promises rejection reasons in the specification. This issue must be further explored and aligned with ongoing discussions in W3C, WHAT WG and ECMA. For example see http://esdiscuss.org/topic/error-objects-in-w3c-apis.

ClaesNilsson commented 9 years ago

See the discussion in https://github.com/sysapps/tcp-udp-sockets/issues/77. The conclusion was that the specification should be walked through regarding error handling to see if any additional error types should be submitted to https://github.com/heycam/webidl/.