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

Support for DTLS and other transport layers, make client async #88

Merged
merged 8 commits into from
Jan 16, 2024

Conversation

osobiehl
Copy link
Contributor

@osobiehl osobiehl commented Jan 5, 2024

features:

  • Client now uses async and tokio UDP sockets as a default
  • Observe now takes ownership of the client to avoid missed packets
  • DTLS is supported using webrtc-rs
  • DTLS is a feature, can be turned off if needed
  • Other backends for transport can be provided by users. They must be able to provide a SockAddr for use with observe, but this could be theoretically hardcoded if you want to use some non-ip protocol
  • Server is reworked to support multiple listeners at the same time
  • Server should no longer block on a single long async request handlers
  • Added tests for DTLS using PSK and PKI
  • Added example for DTLS using PSK
  • Moved all tests to use tokio::test

Still not sure about:

  • handler API takes a Box<CoapRequest>> and returns a Box<CoapRequest>. I did this to avoid copies and still allow for closures as handlers. The ergonomics are not great though
  • handlers for server must be Send + Sync since they can run concurrently
  • naming convention e.g., DTLSConfig vs DtlsConfig

features:
- Client now uses async and tokio
- Observe now takes ownership of the client to avoid missed packets
- DTLS is supported using webrtc-rs
- DTLS is a feature, can be turned off if needed
- Other backends for transport can be provided by users. They must be
  able to provide a SockAddr for use with observe, butthis could be
  theoretically hardcoded if you want to use some non-ip protocol
- Server is reworked to support multiple listeners at the same time
- Server should no longer block on a single long async request handlers
- Added tests for DTLS using PSK and PKI
- Added example for DTLS using PSK
- Moved all tests to use `tokio::test`
@coveralls
Copy link

coveralls commented Jan 5, 2024

Coverage Status

coverage: 74.962% (+1.7%) from 73.312%
when pulling d2db6ae on osobiehl:feature/jb/add-dtls-support
into 773aacf on Covertness:master.

@Covertness
Copy link
Owner

It's a long-awaited improvement of this repo. Thank you for your tremendous help.

@osobiehl
Copy link
Contributor Author

osobiehl commented Jan 8, 2024

hmm, it looks like requests may occasionally fail (udp makes no guarantees for reaching the target, so this is expected). Maybe it would make sense to do retries in the future. I would do that in a separate PR after this is merged though

@Covertness
Copy link
Owner

hmm, it looks like requests may occasionally fail (udp makes no guarantees for reaching the target, so this is expected). Maybe it would make sense to do retries in the future. I would do that in a separate PR after this is merged though

Yeah. Retries would reduce the probability of errors.

@osobiehl osobiehl changed the title [WIP] Support for DTLS and other transport layers, make client async Support for DTLS and other transport layers, make client async Jan 9, 2024
@osobiehl
Copy link
Contributor Author

OK, this looks good to go from my side. Please consider reviewing.
I am also debating rewriting the entire backend to use tower. That way, it is easy to add built-in support for retries/rate limiting. This should not be a breaking change for the future.

Since the changes in this PR are massively breaking, it might also make sense to do some cleanup before doing a major release (e.g., allow options on lowest level of requests)

@Covertness
Copy link
Owner

Good job. I have two questions in the reviewing. Please have a look.

@osobiehl
Copy link
Contributor Author

Good job. I have two questions in the reviewing. Please have a look.

are the comments published? I see no comments

Copy link
Owner

@Covertness Covertness left a comment

Choose a reason for hiding this comment

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

some suggestions

examples/dtls.rs Outdated Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
@Covertness
Copy link
Owner

Good job. I have two questions in the reviewing. Please have a look.

are the comments published? I see no comments

sorry. forgot to submit

@osobiehl
Copy link
Contributor Author

osobiehl commented Jan 15, 2024

ok, changes are implemented. I have also prepared a PR where I add retries to the client over UDP / DTLS if a confirmable message is sent, as well as some basic handling for non-piggybacked responses. I will open it once this is merged, so maybe it would make sense to wait a little before a next release?

@Covertness Covertness merged commit c40af20 into Covertness:master Jan 16, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants