From c3f6ad75e17af4b0b23fb78d1166f5a041b76990 Mon Sep 17 00:00:00 2001 From: Zajcev Evgeny Date: Thu, 16 Feb 2017 00:18:43 +0300 Subject: [PATCH 1/5] Added RFC for network errors handling --- text/0000-network-errors-handling.md | 103 +++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 text/0000-network-errors-handling.md diff --git a/text/0000-network-errors-handling.md b/text/0000-network-errors-handling.md new file mode 100644 index 00000000..187e32f9 --- /dev/null +++ b/text/0000-network-errors-handling.md @@ -0,0 +1,103 @@ +- Feature Name: network errors handling +- Start Date: 2017-02-15 +- RFC PR: (leave this empty) +- Pony Issue: (leave this empty) + +# Summary + +Provide API for handling network errors. + +# Motivation + +Current interfaces, such as `TCPListenNotify`, `TCPConnectionNotify` +and `UDPNotify` do not provide any information about why some action +has failed. This makes it difficult to figure out software +configuration errors. + +# Detailed design + +1. Create a primivite union of possible network errors, similar to + `FileErrNo`, that maps a platform specific errno codes on these + primitive values. + +2. Extend interfaces to accept errors. (New API functions) + +For example: + +```pony +type SocketError is + ( SocketOK + | SocketAccess + | SocketInUse + | SocketConnRefused + | SocketNetUnreach + | SocketTimeout + .... + ) + +interface TCPListenNotify + ... + fun ref listen_error(listen: TCPListener ref, error: SocketError) => + """ + Called if it wasn't possible to bind the listener to an address. + """ + not_listening(listen) + ... + +interface UDPNotify + ... + fun ref listen_error(sock: UDPSocket ref, error: SocketError) => + """ + Called if it wasn't possible to bind the socket to an address. + Default implementation calls + """ + not_listening(listen) + ... + +interface TCPConnectionNotify + ... + fun ref connect_error(conn: TCPConnection ref, error: SocketError) => + """ + Called when we have failed to connect to all possible addresses for the + server. At this point, the connection will never be established. + """ + connect_failed(conn) + ... +``` + +Network code should use new API functions instead of `not_listening` +and `connect_failed`. Default implementation of new API should fall +back to old API calls. This will keep compatibility with existing +network code, but in case you need to handle network error for +yourself, you can override default implementation. + +# How We Teach This + +We could have some annotation for notifier interface functions, such +as `\deprecated\`, so that compiler will yield a warning, if +programmer overrides `\deprecated\` function. In such case developer +will know about API change. + +# How We Test This + +Existing tests for network code already cover the case, since default +implementation falls back to old API. + +# Drawbacks + +Adds a little overhead to handling failures, however network failures +are not so frequent to bother about. Keeping backward compatibility +is more important. + +# Alternatives + +We could have network error stored inside `TCPListener`, `UDPSocket` +and `TCPConnection`, so that notifier is able to extract the error +value from caller on network failure. + +This won't require any addons to notifiers, at the same time it won't +break existing code. + +# Unresolved questions + +How proposed API conforms with [RFC 23](https://github.com/ponylang/rfcs/blob/master/text/0023-network-dont-provide-default-implementation-for-failures.md) From 09518aefa362d03153065f7604048c67fada19a1 Mon Sep 17 00:00:00 2001 From: Zajcev Evgeny Date: Mon, 20 Feb 2017 19:00:21 +0300 Subject: [PATCH 2/5] * 0000-network-errors-handling.md: Motivation update, Design update, keeping notifiers API untouched --- text/0000-network-errors-handling.md | 100 +++++++++++---------------- 1 file changed, 41 insertions(+), 59 deletions(-) diff --git a/text/0000-network-errors-handling.md b/text/0000-network-errors-handling.md index 187e32f9..92e0b844 100644 --- a/text/0000-network-errors-handling.md +++ b/text/0000-network-errors-handling.md @@ -12,7 +12,8 @@ Provide API for handling network errors. Current interfaces, such as `TCPListenNotify`, `TCPConnectionNotify` and `UDPNotify` do not provide any information about why some action has failed. This makes it difficult to figure out software -configuration errors. +configuration errors. Knowledge about nature of network failure is +essential for production software. # Detailed design @@ -20,83 +21,64 @@ configuration errors. `FileErrNo`, that maps a platform specific errno codes on these primitive values. -2. Extend interfaces to accept errors. (New API functions) +```pony + type SocketErrNo is + ( SocketOK + | SocketError + | SocketInUse + | SocketConnRefused + | SocketNetUnreach + | SocketTimeout + ) +``` -For example: + Where: -```pony -type SocketError is - ( SocketOK - | SocketAccess - | SocketInUse - | SocketConnRefused - | SocketNetUnreach - | SocketTimeout - .... - ) - -interface TCPListenNotify - ... - fun ref listen_error(listen: TCPListener ref, error: SocketError) => - """ - Called if it wasn't possible to bind the listener to an address. - """ - not_listening(listen) - ... +* `SocketOK' - success +* `SocketInUse` - local address is already in use +* `SocketConnRefused` - no-one listening on the remote address +* `SocketNetUnreach` - no routing to remote host +* `SocketTimeout` - timeout while attempting connection -interface UDPNotify - ... - fun ref listen_error(sock: UDPSocket ref, error: SocketError) => - """ - Called if it wasn't possible to bind the socket to an address. - Default implementation calls - """ - not_listening(listen) - ... +2. Add private `_errno` field of `SocketErrNo` type into + `TCPConnection`, `UDPSocket` and `TCPListener`. On failure, set + `_errno` using corresponding OS errno code fetched using `SO_ERROR` + socket option + +3. Create publically available method `errno` which returns `_errno`, + for example: -interface TCPConnectionNotify - ... - fun ref connect_error(conn: TCPConnection ref, error: SocketError) => +```pony + fun errno(): SocketErrNo => """ - Called when we have failed to connect to all possible addresses for the - server. At this point, the connection will never be established. + Returns the last error code set for this socket """ - connect_failed(conn) - ... + _errno ``` -Network code should use new API functions instead of `not_listening` -and `connect_failed`. Default implementation of new API should fall -back to old API calls. This will keep compatibility with existing -network code, but in case you need to handle network error for -yourself, you can override default implementation. - # How We Teach This -We could have some annotation for notifier interface functions, such -as `\deprecated\`, so that compiler will yield a warning, if -programmer overrides `\deprecated\` function. In such case developer -will know about API change. +Update the doc strings for `TCPConnectionNotify.connect_failed`, +`UDPNotify.not_listening` and `TCPListenNotify.not_listening` +describing error fetching mechanism described in Design section. + +Code example, showing usage of handling errors, is highly encouraged. # How We Test This -Existing tests for network code already cover the case, since default -implementation falls back to old API. +Add test case of making invalid connection, checking that errno is set +uppon call to notifier. # Drawbacks -Adds a little overhead to handling failures, however network failures -are not so frequent to bother about. Keeping backward compatibility -is more important. +Requires changes in `libponyrt/lang/socket.c` since actual value for +`SO_ERROR` is not exposed to pony level. # Alternatives -We could have network error stored inside `TCPListener`, `UDPSocket` -and `TCPConnection`, so that notifier is able to extract the error -value from caller on network failure. - -This won't require any addons to notifiers, at the same time it won't -break existing code. +Extend notifiers API, so error code is provided uppon call to +notifier. This approach looks not that smooth to integrate as +proposed in Design section. # Unresolved questions From 5f325c3a8f9145a6cbc31b71480632a417b4efaf Mon Sep 17 00:00:00 2001 From: Zajcev Evgeny Date: Mon, 20 Feb 2017 19:04:22 +0300 Subject: [PATCH 3/5] * 0000-network-errors-handling.md: Commentary for `SocketError' added --- text/0000-network-errors-handling.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-network-errors-handling.md b/text/0000-network-errors-handling.md index 92e0b844..85053aee 100644 --- a/text/0000-network-errors-handling.md +++ b/text/0000-network-errors-handling.md @@ -39,6 +39,7 @@ essential for production software. * `SocketConnRefused` - no-one listening on the remote address * `SocketNetUnreach` - no routing to remote host * `SocketTimeout` - timeout while attempting connection +* `SocketError` - other socket error not listed above 2. Add private `_errno` field of `SocketErrNo` type into `TCPConnection`, `UDPSocket` and `TCPListener`. On failure, set From 13af2d4e20e708406bddc528a797ec0b6965cf7c Mon Sep 17 00:00:00 2001 From: Zajcev Evgeny Date: Tue, 21 Feb 2017 10:54:12 +0300 Subject: [PATCH 4/5] * 0000-network-errors-handling.md: set `_errno` for success connections as well --- text/0000-network-errors-handling.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/0000-network-errors-handling.md b/text/0000-network-errors-handling.md index 85053aee..e137531e 100644 --- a/text/0000-network-errors-handling.md +++ b/text/0000-network-errors-handling.md @@ -42,9 +42,9 @@ essential for production software. * `SocketError` - other socket error not listed above 2. Add private `_errno` field of `SocketErrNo` type into - `TCPConnection`, `UDPSocket` and `TCPListener`. On failure, set - `_errno` using corresponding OS errno code fetched using `SO_ERROR` - socket option + `TCPConnection`, `UDPSocket` and `TCPListener`. On connection + outcome, either success or failure, `_errno` must be set using + corresponding OS errno code fetched using `SO_ERROR` socket option 3. Create publically available method `errno` which returns `_errno`, for example: From 8f3e2b1e558dd538a19b786b1ff30bd5031e5d75 Mon Sep 17 00:00:00 2001 From: Zajcev Evgeny Date: Wed, 22 Feb 2017 17:18:35 +0300 Subject: [PATCH 5/5] * 0000-network-errors-handling.md: minor typofix --- text/0000-network-errors-handling.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-network-errors-handling.md b/text/0000-network-errors-handling.md index e137531e..e6bebbf6 100644 --- a/text/0000-network-errors-handling.md +++ b/text/0000-network-errors-handling.md @@ -34,7 +34,7 @@ essential for production software. Where: -* `SocketOK' - success +* `SocketOK` - success * `SocketInUse` - local address is already in use * `SocketConnRefused` - no-one listening on the remote address * `SocketNetUnreach` - no routing to remote host