Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added RFC for network errors handling #78

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 41 additions & 59 deletions text/0000-network-errors-handling.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,91 +12,73 @@ 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

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)
```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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On success, we'd also want to set it back to SocketOK, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, on any outcome (success or failure) we want to set _errno. I'll fix


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

Expand Down