Refactor network code: TCP support, error handling

shamblett / coap

A Coap package for dart

Other

16 stars 13 forks source link

Refactor network code: TCP support, error handling #113

Closed JosefWN closed 2 years ago

JosefWN commented 2 years ago

Branched from https://github.com/shamblett/coap/pull/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.

JosefWN commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

JosefWN commented 2 years ago

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

JKRhb commented 2 years ago

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 commented 2 years ago

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 :)