RustyNova016 / musicbrainz_rs_nova

A wrapper around the musicbrainz API
MIT License
5 stars 3 forks source link

refactor(rate_limit): Remove dependency on tokio #56

Closed Holzhaus closed 2 weeks ago

Holzhaus commented 3 weeks ago

Currently, this crate panics when running in a non-tokio environment.

For example, if you're using futures::executor::ThreadPool, you'll see this error:

thread '<unnamed>' panicked at ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/musicbrainz_rs_nova-0.8.0/src/rate_limit.rs:23:5:
there is no reactor running, must be called from the context of a Tokio 1.x runtime
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

With this change, it will unfortunately still not work because hyper also seems to require tokio, but at least it's a first step for becoming runtime-agnostic.

RustyNova016 commented 3 weeks ago

Well... Yes and no I don't mind making the crate runtime agnostic, and finally removing tokio from being fetched for one function, but I already had a replacement at #54. I finally manage to fix the clippy lints, but need to fix the tests....

As for hyper, it's due to using reqwest for requests. Not sure what replacement could be suitable

Holzhaus commented 2 weeks ago

The documentation page for hyper does not mention tokio, so it does not seem to be a stated goal to depend on it. Maybe we raise the issue upstream.

Anyway, due to these issues I switched back to tokio and use a tokio Runtime instead of a ThreadPool from futures. So this is not really that urgent for me anymore. Just wanted to make you aware of this issue.

RustyNova016 commented 2 weeks ago

The documentation page for hyper does not mention tokio

hyper isn't the issue. cargo tree -i tokio reveals it is used by hyper, which is needed by reqwest, which the later does state that it needs tokio to run

reqwest is only used once in the whole code (or twice if you count feature splits) so it's easy to replace. Just not sure what to replace it with