Proposal: rework top-level API

[JosefWN]

This is an attempt to fix:

• Library semantics #75
• Multiple CoapConfig causes null pointer exception #72
• How is error handling supposed to work? #41

My work was loosely inspired by https://pub.dev/packages/http but with an adaptation to how CoAP clients commonly work in other languages (at least those I like 😅). See the examples to get a feeling for things. TLDR: this PR aims to make the library more native to Dart, to better leverage its strengths. Hopefully I'll manage to make a good case for it below...

Unsure:

• Removed the validate method on the client, I figured either the user builds his request (for example for etag validation, using client.send), or the user relies on the client for building the entire request (for example client.get('hello')). This felt like a bit of a hybrid.

Major stuff:

• There are request builders for the most common verbs (get(...), post(...) etc.), a more manual send(...) for advanced requests and its counterpart observe(...) for streaming, which I suppose is not used often enough to warrant a request builder?
• Tightened up the API by exposing less internals and using the meta package with its @protected annotation for those I couldn't hide completely (will give static analysis warnings). It's now much harder for the user to "do the wrong things".
  • This also reduces the need for some of the error handling that was in place. In fact, we would have needed even more error handling with the old solution, there were plenty of ways to make the client misbehave/crash.
• Major removals:
  • Logging, which is not typically done in libraries (see Dart standard library for example). While the logging helps developers of this library debug, it complicates the code a fair bit, and is an obstacle for users of the library (in fact, all issues mentioned above are touching upon logging and error handling, directly or indirectly). If anything I would suggest adding more integration tests (examples) and working out as many bugs as possible that way. With a really stable API I don't think extensive logging is necessary. I could actually find the issues while working now, with a couple of temporary print() calls.
  • The CoAP.NET project hasn't seen active development since mid-2014 (8 years ago!), and keeping binaries in git is generally discouraged. It's also hard to use these tests on a non-Windows machine. In addition to coap.me, there is Californium, a project which seems to be alive and well, and whose plugtest-server you can run yourself cross-platform. If you miss any tests I propose adding more rather than bringing back the testserver directory, which also contained some duplicate code.
  • The flutter example project felt a bit awkward to keep in a Dart library, only more code to maintain. Flutter users will likely find using the library straight-forward anyhow (it was no problem for me at all).
  • CoapIChannel and CoapIDeliverer and their implementations were seemingly not serving any purpose, even less so after I went from passing the response back to the request, to using the event bus to communicate. This in turn removes the need for the CoapNetworkManager since each client now only has one endpoint (less risk of API user mistakes), as well as the setting poolUdpConnectionsByClient. The pooling doesn't have any drawbacks as far as I can see, the more separation the better?
• Now supports async requests over a shared socket. This required separating different components to a greater degree, and relying less on global/static variables. Two major benefits:

1. The code doesn't crash on:

client.get('large').then((CoapResponse resp) => print(resp));

You can even run multiple observe and different requests concurrently (see examples/get_observe_async.dart).

2. Requests can be batched at far higher efficiency, example:

dart sync_vs_async.dart
Sending 10 async requests...
10 async requests took 49 ms
Sending 10 sync requests...
10 sync requests took 393 ms

Minor stuff:

• Fixes an issue with observe (no endpoint set, _doPrepare called one line too late)
• Fixes an issue where token is not set on the overarching block1 requests (need it for matching in the client)
• Merged the test/issues folder with examples
• Removes notify and error on observe, they were unused arguments from what I could see
• Removes raw_request_creation.dart as that would no longer be supported (no need to worry about preparing the request manually)
• Many other small fixes...

Performance vs 3.5.0 (in parentheses), as baseline before all of my changes:

- 10 sync requests: 362 ms (410 ms)
- Ping: 93 ms (10033 ms) <- returning on reject now, not on timeout
- Blockwise (block2): 799 ms (846 ms) 
- Blockwise (block1): 248 ms (302 ms)

Nothing scientific, just a couple of quick checks to ensure my changes that went out in v3.6.0 and in this PR are not making things slower.

[JosefWN]

Ok! Ready for review @shamblett, @JKRhb! Linting, formatting & tests are passing, examples are working :)

Not sure how to go about reviewing what turned out to be a gargantuan PR, sorry for that. Perhaps the best is to just start with viewing and running the examples to get a feel for the API. Everything is tested except for multicast, didn't have a good way of testing it.

[JKRhb]

Thank you for your hard work, @JosefWN!

Not sure how to go about reviewing what turned out to be a gargantuan PR, sorry for that.

Maybe aspects like the test server and flutter example removal could be spun off into its own PR? That would reduce the size of the PR a bit

[JKRhb]

I think both the CoapMessage and the CoapRequest constructors could be simplified a bit. (This requires some more changes at other places in the code base.)

[JKRhb]

LGTM :) @shamblett Can you also have a look on this PR? :)

[JKRhb]

Oh, one last thing: I think we could also use Object.hash() in places like this one (here it would probably be simply return Object.hash(_type)).

[JosefWN]

Ah, nice catch. In addition, if all fields are included in Object.hash(), I don't think we need to override hashCode or operator ==?

[JosefWN]

Okay, I found a couple more places to clean up while I was at it: fcc5f66

With client mapping 1:1 to a socket we don't need CoapKeyUri, we can match blockwise requests on token. Having a List<Map<int?, CoapExchange>> for matching on id and a Map<String, CoapExchange> for matching on token works just fine. There is no need for the other two wrapping classes.

I first noticed the CoapObserveNotificationOrderer had an odd getNextObserveNumber method, then I realized the entire class was not used at all. Removed it for now, confusing with dead code.

Re-ran all examples and everything works.

[JKRhb]

I guess here we could use something like this?

Suggested change

	_hash = _uri.hashCode * 31 + (_endpoint?.hashCode ?? 0);
	_hash = Object.hash(_uri, _endpoint);

[shamblett]

I've merged this to its own branch for now, issue75, while I check it out.

One question about logging, the original logging was really supplied so users could just turn on logging and supply me with a log of what happened when they raised an issue, without this we are pretty blind. The old logging although as you say problematic in itself did allow post mortem debugging for issue investigation. Are we now saying we are just replacing the old logging with print statements? This is OK but we mustn't lose detail here, the examples now provide minimal logging(OK these of course work so that maybe why).

Will we still be able to see message detail, i.e token's/sequence numbers/flags/block counts etc. that we used to do?
These may be invaluable,especially in cases where the CoAP server the user is using doesn't work properly(we have had this in the past). We may not be able to reproduce the fault, nor give enough detail to the user as to what the problem is.

Sorry if I've missed something here.

[JosefWN]

Hi @shamblett, and sorry for the large PR!

Personally I'm a big fan of the event bus since it helps us not only decouple the components within the library, but also gives us the opportunity to introspect the request/response flow. In this PR I always fire events (even if there is a hook) for this purpose. I'm also always including data with all events, so we can differentiate between multiple concurrent requests.

To make debug logging easier, we could expose the bus on the client as well, and add toString methods on the events. With that in place, I think what you are asking for could be achieved with this one-liner:

client.events.on().listen(print);

Example: JosefWN@3a9dcaa

If you try running the log_events.dart example I included in the commit, you will see all blockwise activity.

This also makes it possible for library users to listen for and act on any events they choose, without imposing a specific logger or event handler on them (of course, we would still handle the events internally as required, but we wouldn't programmatically obscure anything from the library user). If we are missing important events for debugging we can just add more of them. What do you think?

Another thing we could do when users encounter issues is to ask them to provide us with a minimal example reproducing that issue. Shifting the burden of proof to the person with the problem will reduce the debugging load on us even further, as we could just run their problematic example code, or a very similar piece of code, against another server.

[shamblett]

This sounds good to me, the existing logging did need to go, I'll have a look at log_events.dart but I'm happy with this approach.