Refactor network code: TCP support, error handling

[JosefWN]

Branched from #111 (so focus on the second commit for this PR), improve the error handling in order to fix #110:

• Removes needless async calls to reduce risk of errors not bubbling up (catchError vs try/catch), the code is also a bit easier to follow when it's only async where it really needs to be.
• Re-creates the socket onDone (at this point we can't listen on it again), I'm assuming the socket is closed when an unrecoverable error occurs.
• Sends an error on the event bus that we can listen for when an error on the socket occurs, and a rebind event when the socket is rebound.
• Tried with both polling and observe, and the code can recover from a lost connection.
• Haven't tested the DTLS part of the code.

Would appreciate if you guys have time to test another round that everything seems okay! Perhaps it's easiest once the other PR's are merged and this PR has been rebased to include those fixes.

[JKRhb]

Changes look good to me in general :) I think there might be some refactoring potential, though (see below).

One thing, I noticed, though: Running the examples with the current PR changes present causes them to not terminate. Not sure why that is the case :/

Diff with suggestions

diff --git a/lib/src/network/coap_inetwork.dart b/lib/src/network/coap_inetwork.dart
index c426ea2..2e2e8cd 100644
--- a/lib/src/network/coap_inetwork.dart
+++ b/lib/src/network/coap_inetwork.dart
@@ -5,10 +5,12 @@
  * Copyright :  S.Hamblett
  */
 
+import 'package:meta/meta.dart';
 import 'package:typed_data/typed_data.dart';
 
 import '../coap_config.dart';
 import '../coap_constants.dart';
+import '../event/coap_event_bus.dart';
 import '../net/coap_internet_address.dart';
 import 'coap_network_openssl.dart';
 import 'coap_network_tinydtls.dart';
@@ -55,6 +57,9 @@ class CoapDtlsException implements Exception {
 /// Abstract networking class, allows different implementations for
 /// UDP, test etc.
 abstract class CoapINetwork {
+  @protected
+  CoapEventBus get eventBus;
+
   /// The internet address
   CoapInternetAddress get address;
 
@@ -153,4 +158,14 @@ abstract class CoapINetwork {
         );
     }
   }
+
+  Future<void> rebind() async {
+    try {
+      close();
+      await bind();
+    } on Exception catch (_) {
+      // Fail silently (try again on next request)
+    }
+    eventBus.fire(CoapSocketRebindEvent());
+  }
 }
diff --git a/lib/src/network/coap_network_openssl.dart b/lib/src/network/coap_network_openssl.dart
index 270387e..50c5dea 100644
--- a/lib/src/network/coap_network_openssl.dart
+++ b/lib/src/network/coap_network_openssl.dart
@@ -17,7 +17,7 @@ import '../net/coap_internet_address.dart';
 import 'coap_inetwork.dart';
 
 /// DTLS network using OpenSSL
-class CoapNetworkOpenSSL implements CoapINetwork {
+class CoapNetworkOpenSSL extends CoapINetwork {
   /// Initialize with an [address] and a [port].
   ///
   /// This [CoapINetwork] can be configured to be used [withTrustedRoots] and
@@ -32,19 +32,19 @@ class CoapNetworkOpenSSL implements CoapINetwork {
     required final bool withTrustedRoots,
     final String namespace = '',
     final String? ciphers,
-  })  : _eventBus = CoapEventBus(namespace: namespace),
+  })  : eventBus = CoapEventBus(namespace: namespace),
         _ciphers = ciphers,
         _verify = verify,
         _withTrustedRoots = withTrustedRoots;
 
-  final CoapEventBus _eventBus;
+  final CoapEventBus eventBus;
 
   void _processFrame(final Uint8List frame) {
     final buff = Uint8Buffer();
     if (frame.isNotEmpty) {
       buff.addAll(frame.toList());
       final rxEvent = CoapDataReceivedEvent(buff, address);
-      _eventBus.fire(rxEvent);
+      eventBus.fire(rxEvent);
     }
   }
 
@@ -67,7 +67,7 @@ class CoapNetworkOpenSSL implements CoapINetwork {
   final int port;
 
   @override
-  String get namespace => _eventBus.namespace;
+  String get namespace => eventBus.namespace;
 
   bool get bound => _socket != null;
 
@@ -107,7 +107,7 @@ class CoapNetworkOpenSSL implements CoapINetwork {
       },
       // ignore: avoid_types_on_closure_parameters
       onError: (final Object e, final StackTrace s) =>
-          _eventBus.fire(CoapSocketErrorEvent(e, s)),
+          eventBus.fire(CoapSocketErrorEvent(e, s)),
       // Socket stream is done and can no longer be listened to
       onDone: rebind,
     );
@@ -150,14 +150,4 @@ class CoapNetworkOpenSSL implements CoapINetwork {
     _dtlsConnection?.free();
     _socket = null;
   }
-
-  Future<void> rebind() async {
-    try {
-      close();
-      await bind();
-    } on Exception catch (_) {
-      // Fail silently (try again on next request)
-    }
-    _eventBus.fire(CoapSocketRebindEvent());
-  }
 }
diff --git a/lib/src/network/coap_network_tinydtls.dart b/lib/src/network/coap_network_tinydtls.dart
index a5a558e..30e2e9f 100644
--- a/lib/src/network/coap_network_tinydtls.dart
+++ b/lib/src/network/coap_network_tinydtls.dart
@@ -58,7 +58,7 @@ EcdsaKeys? _createTinyDtlsEcdsaKeys(final internal.EcdsaKeys? coapEcdsaKeys) {
 }
 
 /// DTLS network using dart_tinydtls
-class CoapNetworkTinyDtls implements CoapINetwork {
+class CoapNetworkTinyDtls extends CoapINetwork {
   /// Initialize with an [address] and a [port] as well as credentials.
   ///
   /// These credentials can either be provided by a [pskCredentialsCallback]
@@ -79,9 +79,9 @@ class CoapNetworkTinyDtls implements CoapINetwork {
   })  : _tinydtlsPskCallback =
             _createTinyDtlsPskCallback(pskCredentialsCallback),
         _ecdsaKeys = _createTinyDtlsEcdsaKeys(ecdsaKeys),
-        _eventBus = CoapEventBus(namespace: namespace);
+        eventBus = CoapEventBus(namespace: namespace);
 
-  final CoapEventBus _eventBus;
+  final CoapEventBus eventBus;
 
   final PskCallback? _tinydtlsPskCallback;
 
@@ -96,7 +96,7 @@ class CoapNetworkTinyDtls implements CoapINetwork {
   final int port;
 
   @override
-  String get namespace => _eventBus.namespace;
+  String get namespace => eventBus.namespace;
 
   bool _bound = false;
   bool get bound => _bound;
@@ -122,12 +122,12 @@ class CoapNetworkTinyDtls implements CoapINetwork {
           final coapAddress =
               CoapInternetAddress(datagram.address.type, datagram.address);
           final rxEvent = CoapDataReceivedEvent(buff, coapAddress);
-          _eventBus.fire(rxEvent);
+          eventBus.fire(rxEvent);
         }
       },
       // ignore: avoid_types_on_closure_parameters
       onError: (final Object e, final StackTrace s) =>
-          _eventBus.fire(CoapSocketErrorEvent(e, s)),
+          eventBus.fire(CoapSocketErrorEvent(e, s)),
       // Socket stream is done and can no longer be listened to
       onDone: rebind,
     );
@@ -157,14 +157,4 @@ class CoapNetworkTinyDtls implements CoapINetwork {
   void close() {
     _dtlsClient?.close();
   }
-
-  Future<void> rebind() async {
-    try {
-      close();
-      await bind();
-    } on Exception catch (_) {
-      // Fail silently (try again on next request)
-    }
-    _eventBus.fire(CoapSocketRebindEvent());
-  }
 }
diff --git a/lib/src/network/coap_network_udp.dart b/lib/src/network/coap_network_udp.dart
index fd43386..c520b3d 100644
--- a/lib/src/network/coap_network_udp.dart
+++ b/lib/src/network/coap_network_udp.dart
@@ -14,20 +14,21 @@ import '../net/coap_internet_address.dart';
 import 'coap_inetwork.dart';
 
 /// UDP network
-class CoapNetworkUDP implements CoapINetwork {
+class CoapNetworkUDP extends CoapINetwork {
   /// Initialize with an address and a port
   CoapNetworkUDP(this.address, this.port, {final String namespace = ''})
-      : _eventBus = CoapEventBus(namespace: namespace);
+      : eventBus = CoapEventBus(namespace: namespace);
 
   CoapNetworkUDP.from(
     final CoapNetworkUDP src, {
     required final String namespace,
   })  : address = src.address,
         port = src.port,
-        _eventBus = CoapEventBus(namespace: namespace),
+        eventBus = CoapEventBus(namespace: namespace),
         _socket = src.socket;
 
-  final CoapEventBus _eventBus;
+  @override
+  final CoapEventBus eventBus;
 
   @override
   final CoapInternetAddress address;
@@ -36,7 +37,7 @@ class CoapNetworkUDP implements CoapINetwork {
   final int port;
 
   @override
-  String get namespace => _eventBus.namespace;
+  String get namespace => eventBus.namespace;
 
   /// UDP socket
   RawDatagramSocket? _socket;
@@ -69,7 +70,7 @@ class CoapNetworkUDP implements CoapINetwork {
                 buff.addAll(d.data.toList());
                 final coapAddress =
                     CoapInternetAddress(d.address.type, d.address);
-                _eventBus.fire(CoapDataReceivedEvent(buff, coapAddress));
+                eventBus.fire(CoapDataReceivedEvent(buff, coapAddress));
               }
             }
             break;
@@ -83,7 +84,7 @@ class CoapNetworkUDP implements CoapINetwork {
       },
       // ignore: avoid_types_on_closure_parameters
       onError: (final Object e, final StackTrace s) =>
-          _eventBus.fire(CoapSocketErrorEvent(e, s)),
+          eventBus.fire(CoapSocketErrorEvent(e, s)),
       // Socket stream is done and can no longer be listened to
       onDone: rebind,
     );
@@ -106,14 +107,4 @@ class CoapNetworkUDP implements CoapINetwork {
     _socket?.close();
     _socket = null;
   }
-
-  Future<void> rebind() async {
-    try {
-      close();
-      await bind();
-    } on Exception catch (_) {
-      // Fail silently (try again on next request)
-    }
-    _eventBus.fire(CoapSocketRebindEvent());
-  }
 }

[JKRhb]

I think we could move this method to CoapINetwork to avoid code duplication (see the patch in the comment above).

[JosefWN]

I think we might need a broader refactor, but that might be better saved for another PR. For example, I don't think you just bind for TCP (since it's not a connectionless protocol you connect), so when we support TCP this method might be a bit out-of-place on the top-level. I would use more generic names at the top level, and more specific names below.

A DTLS socket is also a UDP socket at the end of the day, I would rename CoapNetworkOpenSSL and perhaps also CoapNetworkTinyDtls (although implied since DTLS is UDP) to indicate that they are UDP networks/sockets, and have them extend CoapNetworkUDP.

This way, CoapNetworkUDP manages all raw UDP sockets, and this socket is passed into the DTLS clients when they are created (which is also possible with tinydtls). What do you think?

[JKRhb]

Very good points! Sounds like a good approach to me :)

[JosefWN]

One thing, I noticed, though: Running the examples with the current PR changes present causes them to not terminate. Not sure why that is the case :/

Argh, though I tested this by looking for a rebind event on the event bus when we purposely close the socket (for example on client close), I suspect the rebind is taking place when we try to finally close the socket as well. Will fix!

[JosefWN]

Extended CoapNetworkUDP for DTLS in the end, and made close take an argument rebind.

If the network is down when the app starts you will get an exception:

dart put_resource.dart
Sending put /create1 to coap.me
CoAP encountered an exception: SocketException: Failed host lookup: 'coap.me' (OS Error: nodename nor servname provided, or not known, errno = 8)

But if the network is up and goes down it will automatically attempt to reconnect continuously. Couldn't get rid of the rebind methods since just calling super.rebind in the subclasses would just rebind the socket, not the DTLS connection.

It's not super pretty, but the net effect of the change (despite some duplicate code) is only 27 lines... I don't know, what do you think @JKRhb? Can't come up with a really pretty way to handle this.

[JKRhb]

I think the changes look good for now :) Maybe we can revisit the structure once better/native DTLS support becomes available.

[JosefWN]

Decided to work a bit more on this after a good nights sleep...

[JKRhb]

Great changes! (Especially regarding TCP support \o/)

Just a quick comment: I think the addressType could probably be derived from the destination address?

By the way, should we change the default addressType for lookups to InternetAddressType.any? Or could this cause problems for environments where IPv6 is unavailable?

[JosefWN]

Good points, fixed in my latest commit! I think InternetAddressType.any should be fine? Worst-case it can be overridden by the user. Changing didn't cause any problems for me at least.

Still need to test things out a bit more, especially the retry logic for TCP. IIRC futures (including timers) are permanently killed when an app goes to sleep in Flutter for example, at least on iOS. On the other hand, perhaps then the entire CoapClient should be re-created? In my Flutter app I'm listening for AppLifecycleState.resumed to get things going again when the app resumes from sleep. It might not be something for this library to worry about I mean (other than perhaps mentioning it in the README).

I'm also firing a CoapDataReceivedEvent even if d.data.isEmpty now, mainly for debugging (if we receive empty datagrams from the server it could be good to know). Need to test that it doesn't break anything.

[JosefWN]

Fixed some potential issues, updated the README to include a note on futures in Flutter (and to make it a bit less dense). Squashed the commits.

If you can, please try the DTLS code to see that it works okay (best of all by cutting the connection during requests, for example using the code snippet in #110, and then turning it back on again... can take a little while before it comes back up).

[JosefWN]

A bit verbose naming, but when we have one DTLS solution for all cases in the future we could call it CoapNetworkDTLS instead, which implies UDP clearly.

[JKRhb]

Since the Network classes are only used internally, maybe we could also use names like DtlsNetworkOpenSSL (or NetworkDtlsOpenSSL)?

[JosefWN]

Agreed, I made a separate issue so we could go over the internal classes and do it to all of them to keep things streamlined: #121

[JKRhb]

Sorry @JosefWN, I only managed to test your PR now, and it seems to work fine with both UDP and DTLS :) Could you also have a look, @shamblett? (Also on the other PRs?)

[JosefWN]

Sorry @JosefWN, I only managed to test your PR now, and it seems to work fine with both UDP and DTLS :)

No worries! Been quite busy myself :)