Skip to content

use minreq instead of jsonrpc http implementation #259

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

Closed
wants to merge 1 commit into from

Conversation

conorpp
Copy link

@conorpp conorpp commented Nov 29, 2022

The jsonrpc http implementation is hand rolled and has a number of "basic" http issues that we shouldn't have to deal with (missing common headers, content-length parsing issue, etc).

I replaced it with the simple most popular rust HTTP client, reqwest.

@apoelstra
Copy link
Member

This increases the number of dependencies from 44 to 180 :) it is completely dead-on-arrival for that reason alone.

I appreciate the frustration with "basic" HTTP issues but for some reason the Rust ecosystem does not have a single HTTP client crate that can do simple things (like exchanging ASCII-encoded JSON with a daemon we can trust to be reasonably well behaved) without pulling in 100+ dependencies.

If you have specific issues with rust-jsonrpc we'd really appreciate bug reports. At some point we do intend for it to be a solid, if not full-featured, HTTP client, but we have limited usage data to work with.

@conorpp
Copy link
Author

conorpp commented Nov 30, 2022

How about https://github.com/algesten/ureq? It would bring dependencies to like 80.

I think to make a good substitute just relying on bug requests over time would be painful. To do it right I think would be to make a dedicated http crate with a suite of HTTP tests. A lot of work, much more simple to use an existing implementation.

@conorpp
Copy link
Author

conorpp commented Nov 30, 2022

Even better, minireq: only 1-2 deps pulled in.

@apoelstra
Copy link
Member

Nice, thanks! We'll take a look.

@conorpp conorpp changed the title use reqwest instead of jsonrpc http implementation use minreq instead of jsonrpc http implementation Nov 30, 2022
@conorpp
Copy link
Author

conorpp commented Nov 30, 2022

updated

@apoelstra
Copy link
Member

At some point later today I will PR to minreq with a few fixes:

  • They recently broke compilation with the proxy feature by adding some Eq derives inconsistently (at clippy's advice, ironically)
  • A couple feature gates are incompatible and can't be used simultaneously, which we would need to fix to use it as a dependency (otherwise our own builds wouldn't reliably compile)
  • They have no MSRV, but it looks like every feature but rustls compiles with Rustc 1.48.0, which is fine by us ... but we would want them to have a MSRV policy that's checked in CI, and maybe a promise not to change it by accident.

If the maintainers are friendly and accepting of these changes I see no reason not to use it here :)

@TheBlueMatt
Copy link
Member

I'd think they need to also be open to an optional-async feature - if not we'll probably have to implement our own HTTP client anyway one way or another, and I'm not sure why we'd bother with multiple options for HTTP clients.

@tcharding
Copy link
Member

We added minreq support to jsonrpc in apoelstra/rust-jsonrpc#94, should we use the minreq_http feature instead of this now?

@tcharding
Copy link
Member

Replaced by #341

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.

5 participants