search

FlorianUekermann / rustls-acme

Apache License 2.0
144 stars 28 forks source link

refactor: introduce full tokio compatability #44

Closed dignifiedquire closed 1 year ago

dignifiedquire commented 1 year ago

Removes async-std/smol specific deps when using the tokio feature, and uses the tokio specific versions of the deps when the tokio features is enabeld.

FlorianUekermann commented 1 year ago

Hi, thank you for the PR. Removing the async-h1 and async-std dependency is the most important todo for this crate. However, I'm hesitant to move forward with the approach taken here for a few reasons:

  1. Changing any type parameters, field types, function argument types or return types based on features is a no-go. Because:
    1. It makes the default docs invalid if non-default features are enabled
    2. Feature selection isn't entirely user controlled, but the union of enabled features in all dependencies that depend on rustls-acme.
    3. Not likely, but if a user wants to use multiple variants of interfaces the library offers, there is no technical reason to prevent this.
  2. I don't want to split code paths unnecessarily. That increases the chance for errors, makes development more tedious and reduces the implicit test coverage provided by users. Automated testing to mitigate this issue is tricky for this crate and I for one prefer confidence due to low complexity over higher complexity mitigated by more tests anyway.
  3. I am targeting the "sort-of standard" traits from the futures crate. I think that is what libraries should try to do nowadays. I'm happy to add feature gated helpers to make life easier for tokio users (or frameworks like axum), but only on the outside wrapping the default implementation.

I'm admittedly a bit slow at resolving #19, because it seems like that entails implementing a tiny futures based http client, that does not depend on either async-std or tokio. I'm working on it though... I'm open to suggestions or help on that front. I haven't found any popular ones that meet those criteria.

I realize that there is also the question of the underlying method that triggers wakers for timers and network connections. I tend to choose the async-io crate for that. That isn't a perfect solution, because some runtimes choose other crates or have their own implementation. But realistically it is a good lightweight solution and whether there are one or two threads running for that purpose doesn't seem like a big deal. I think this may be a "perfect is the enemy of good" situation for now. On that front I'm content waiting for ecosystem wide consolidation or a some high-level provider trait to emerge as quasi standard.

dignifiedquire commented 1 year ago

That's fair, unfortunately I waited for multiple years for any standardisation to happen and have somewhat given up on this. As I do need to get my dependency tree down, I have published a version that only supports tokio for now here: https://github.com/n0-computer/tokio-rustls-acme

dignifiedquire commented 1 year ago

I'm admittedly a bit slow at resolving #19, because it seems like that entails implementing a tiny futures based http client, that does not depend on either async-std or tokio. I'm working on it though... I'm open to suggestions or help on that front. I haven't found any popular ones that meet those criteria.

That is pretty painful, last time I tried. I worked on surf for a while, which originally supported hyper/tokio and async-std/async-h1. Abstracting over these was quite complex and painful, which is why surf now doesn't support hyper anymore.