From e5bc69d7603fd864cff126ffc580a2cfabee5a16 Mon Sep 17 00:00:00 2001 From: kcrimson Date: Thu, 20 Apr 2017 22:44:30 +0200 Subject: [PATCH] rfc #23: Prevent Pony users from creating "silent failure" scenarios with their network code. TCPConnectionNotify, UDPNotify and TCPListenNotify will all, by default, silently eat connection failures. --- examples/echo/echo.pony | 3 +++ examples/net/server.pony | 3 +++ packages/net/_test.pony | 22 +++++++++++++++++---- packages/net/http/_server_conn_handler.pony | 6 ++++++ packages/net/tcp_connection.pony | 21 ++++++++++++++------ packages/net/tcp_connection_notify.pony | 11 +++++++++-- packages/net/tcp_listen_notify.pony | 11 +++++++++-- packages/net/tcp_listener.pony | 9 ++++++--- packages/net/udp_notify.pony | 11 +++++++++-- packages/net/udp_socket.pony | 5 +++++ 10 files changed, 83 insertions(+), 19 deletions(-) diff --git a/examples/echo/echo.pony b/examples/echo/echo.pony index 4f70c5c123..c95b89ce08 100644 --- a/examples/echo/echo.pony +++ b/examples/echo/echo.pony @@ -51,3 +51,6 @@ class Server is TCPConnectionNotify fun ref closed(conn: TCPConnection ref) => _out.print("server closed") + + fun ref connect_failed(conn: TCPConnection ref) => + _out.print("connect failed") diff --git a/examples/net/server.pony b/examples/net/server.pony index bc313068fa..57a616455c 100644 --- a/examples/net/server.pony +++ b/examples/net/server.pony @@ -22,3 +22,6 @@ class ServerSide is TCPConnectionNotify fun ref closed(conn: TCPConnection ref) => _env.out.print("server closed") + + fun ref connect_failed(conn: TCPConnection ref) => + _env.out.print("connect failed") diff --git a/packages/net/_test.pony b/packages/net/_test.pony index d7d573d01a..24cda84301 100644 --- a/packages/net/_test.pony +++ b/packages/net/_test.pony @@ -214,7 +214,7 @@ class _TestTCPExpectNotify is TCPConnectionNotify _send(conn, "hi there") fun ref connect_failed(conn: TCPConnection ref) => - _h.fail_action("client connect") + _h.fail_action("client connect failed") fun ref connected(conn: TCPConnection ref) => _h.complete_action("client connect") @@ -295,7 +295,7 @@ class _TestTCPWritevNotifyClient is TCPConnectionNotify conn.writev(recover ["hello"; ", hello"] end) fun ref connect_failed(conn: TCPConnection ref) => - _h.fail_action("client connect") + _h.fail_action("client connect failed") class _TestTCPWritevNotifyServer is TCPConnectionNotify let _h: TestHelper @@ -318,6 +318,9 @@ class _TestTCPWritevNotifyServer is TCPConnectionNotify end true + fun ref connect_failed(conn: TCPConnection ref) => + _h.fail_action("sender connect failed") + class iso _TestTCPMute is UnitTest """ Test that the `mute` behavior stops us from reading incoming data. The @@ -370,6 +373,10 @@ class _TestTCPMuteReceiveNotify is TCPConnectionNotify _h.complete(false) true + fun ref connect_failed(conn: TCPConnection ref) => + _h.fail_action("receiver connect failed") + + class _TestTCPMuteSendNotify is TCPConnectionNotify """ Notifier that sends data back when it receives any. Used in conjunction with @@ -388,7 +395,7 @@ class _TestTCPMuteSendNotify is TCPConnectionNotify _h.complete_action("sender connected") fun ref connect_failed(conn: TCPConnection ref) => - _h.fail_action("sender connected") + _h.fail_action("sender connect failed") fun ref received(conn: TCPConnection ref, data: Array[U8] val, times: USize): Bool @@ -448,6 +455,10 @@ class _TestTCPUnmuteReceiveNotify is TCPConnectionNotify _h.complete(true) true + fun ref connect_failed(conn: TCPConnection ref) => + _h.fail_action("receiver connect failed") + + class iso _TestTCPThrottle is UnitTest """ Test that when we experience backpressure when sending that the `throttled` @@ -493,6 +504,9 @@ class _TestTCPThrottleReceiveNotify is TCPConnectionNotify _h.complete_action("receiver asks for data") _h.dispose_when_done(conn) + fun ref connect_failed(conn: TCPConnection ref) => + _h.fail_action("receiver connect failed") + class _TestTCPThrottleSendNotify is TCPConnectionNotify """ Notifier that sends data back when it receives any. Used in conjunction with @@ -512,7 +526,7 @@ class _TestTCPThrottleSendNotify is TCPConnectionNotify _h.complete_action("sender connected") fun ref connect_failed(conn: TCPConnection ref) => - _h.fail_action("sender connected") + _h.fail_action("sender connect failed") fun ref received(conn: TCPConnection ref, data: Array[U8] val, times: USize): Bool diff --git a/packages/net/http/_server_conn_handler.pony b/packages/net/http/_server_conn_handler.pony index 35b99e0e0f..543cbc1c90 100644 --- a/packages/net/http/_server_conn_handler.pony +++ b/packages/net/http/_server_conn_handler.pony @@ -92,3 +92,9 @@ class _ServerConnHandler is TCPConnectionNotify try (_session as _ServerConnection).cancel(Payload.request()) end + + fun ref connect_failed(conn: TCPConnection ref) => + """ + The connect has failed. TODO: is it a case for server-side? + """ + None diff --git a/packages/net/tcp_connection.pony b/packages/net/tcp_connection.pony index 2fb213d209..e45595fdfa 100644 --- a/packages/net/tcp_connection.pony +++ b/packages/net/tcp_connection.pony @@ -36,6 +36,9 @@ actor TCPConnection _out.print("GOT:" + String.from_array(consume data)) conn.close() + fun ref connect_failed(conn: TCPConnection ref) => + None + actor Main new create(env: Env) => try @@ -84,6 +87,9 @@ actor TCPConnection fun ref unthrottled(connection: TCPConnection ref) => _coordinator.unthrottled(this) + fun ref connect_failed(conn: TCPConnection ref) => + None + actor Coordinator var _senders: List[Any tag] = _senders.create() @@ -116,18 +122,21 @@ actor TCPConnection "" end - fun ref sentv(conn: TCPConnection ref, data: ByteSeqIter): ByteSeqIter => - if not _throttled then - data - else - Array[String] - end + fun ref sentv(conn: TCPConnection ref, data: ByteSeqIter): ByteSeqIter => + if not _throttled then + data + else + Array[String] + end fun ref throttled(connection: TCPConnection ref) => _throttled = true fun ref unthrottled(connection: TCPConnection ref) => _throttled = false + + fun ref connect_failed(conn: TCPConnection ref) => + None ``` In general, unless you have a very specific use case, we strongly advise that diff --git a/packages/net/tcp_connection_notify.pony b/packages/net/tcp_connection_notify.pony index 28a636cdd9..5deb770616 100644 --- a/packages/net/tcp_connection_notify.pony +++ b/packages/net/tcp_connection_notify.pony @@ -26,12 +26,19 @@ interface TCPConnectionNotify """ None - fun ref connect_failed(conn: TCPConnection ref) => + fun ref connect_failed(conn: TCPConnection ref) """ Called when we have failed to connect to all possible addresses for the server. At this point, the connection will never be established. + + It is expected to implement proper error handling. You need to opt in to + ignoring errors, which can be implemented like this: + + ``` + fun ref connect_failed(conn: TCPConnection ref) => + None + ``` """ - None fun ref auth_failed(conn: TCPConnection ref) => """ diff --git a/packages/net/tcp_listen_notify.pony b/packages/net/tcp_listen_notify.pony index d43f90c403..f528fc46d0 100644 --- a/packages/net/tcp_listen_notify.pony +++ b/packages/net/tcp_listen_notify.pony @@ -11,11 +11,18 @@ interface TCPListenNotify """ None - fun ref not_listening(listen: TCPListener ref) => + fun ref not_listening(listen: TCPListener ref) """ Called if it wasn't possible to bind the listener to an address. + + It is expected to implement proper error handling. You need to opt in to + ignoring errors, which can be implemented like this: + + ``` + fun ref not_listening(listen: TCPListener ref) => + None + ``` """ - None fun ref closed(listen: TCPListener ref) => """ diff --git a/packages/net/tcp_listener.pony b/packages/net/tcp_listener.pony index 335f277d94..142faa5709 100644 --- a/packages/net/tcp_listener.pony +++ b/packages/net/tcp_listener.pony @@ -14,6 +14,9 @@ actor TCPListener fun ref received(conn: TCPConnection ref, data: Array[U8] iso): Bool => conn.write(String.from_array(consume data)) true + fun ref connect_failed(conn: TCPConnection ref) => + None + class MyTCPListenNotify is TCPListenNotify fun ref connected(listen: TCPListener ref): TCPConnectionNotify iso^ => @@ -38,7 +41,7 @@ actor TCPListener var _max_size: USize new create(auth: TCPListenerAuth, notify: TCPListenNotify iso, - host: String = "", service: String = "0", limit: USize = 0, + host: String = "", service: String = "0", limit: USize = 0, init_size: USize = 64, max_size: USize = 16384) => """ @@ -54,7 +57,7 @@ actor TCPListener _notify_listening() new ip4(auth: TCPListenerAuth, notify: TCPListenNotify iso, - host: String = "", service: String = "0", limit: USize = 0, + host: String = "", service: String = "0", limit: USize = 0, init_size: USize = 64, max_size: USize = 16384) => """ @@ -70,7 +73,7 @@ actor TCPListener _notify_listening() new ip6(auth: TCPListenerAuth, notify: TCPListenNotify iso, - host: String = "", service: String = "0", limit: USize = 0, + host: String = "", service: String = "0", limit: USize = 0, init_size: USize = 64, max_size: USize = 16384) => """ diff --git a/packages/net/udp_notify.pony b/packages/net/udp_notify.pony index a4f55c1a4a..2aec76bd27 100644 --- a/packages/net/udp_notify.pony +++ b/packages/net/udp_notify.pony @@ -11,11 +11,18 @@ interface UDPNotify """ None - fun ref not_listening(sock: UDPSocket ref) => + fun ref not_listening(sock: UDPSocket ref) """ Called if it wasn't possible to bind the socket to an address. + + It is expected to implement proper error handling. You need to opt in to + ignoring errors, which can be implemented like this: + + ``` + fun ref not_listening(sock: UDPSocket ref) => + None + ``` """ - None fun ref received(sock: UDPSocket ref, data: Array[U8] iso, from: NetAddress) => diff --git a/packages/net/udp_socket.pony b/packages/net/udp_socket.pony index b32e1a03b8..beaba72897 100644 --- a/packages/net/udp_socket.pony +++ b/packages/net/udp_socket.pony @@ -22,6 +22,9 @@ actor UDPSocket => sock.write(consume data, from) + fun ref not_listening(sock: UDPSocket ref) => + None + actor Main new create(env: Env) => try @@ -47,6 +50,8 @@ actor UDPSocket => _out.print("GOT:" + String.from_array(consume data)) sock.dispose() + fun ref not_listening(sock: UDPSocket ref) => + None actor Main new create(env: Env) =>