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

HTTP client should not try to parse body of response with non-2XX status code #1359

Closed
xoac opened this issue Sep 25, 2023 · 4 comments · Fixed by #1361
Closed

HTTP client should not try to parse body of response with non-2XX status code #1359

xoac opened this issue Sep 25, 2023 · 4 comments · Fixed by #1361

Comments

@xoac
Copy link

xoac commented Sep 25, 2023

What went wrong?

Concurrent request to tx_search fails with value: serde parse error

    Finished dev [unoptimized + debuginfo] target(s) in 6.36s
     Running `target/debug/tendermint_tx_search`
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: serde parse error

Caused by:
    expected value at line 1 column 1

Steps to reproduce

Cargo.toml
[package]
name = "tendermint_tx_search"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
# async 
tokio = { version = "1.13.1", features = [
    "rt-multi-thread",
    "time",
    "sync",
    "macros",
] }

# tendermint
tendermint-rpc = { version = "0.33", features = ["http-client"] }
tendermint = { version = "0.33" }
main.rs (not working)
use tendermint_rpc::{client::Client, query::Query, Order};

#[tokio::main]
async fn main() {
    let mut c = tendermint_rpc::client::HttpClient::new("https://juno-rpc.icycro.org").unwrap();
    c.set_compat_mode(tendermint_rpc::client::CompatMode::V0_37);

    let query = Query::default()
        .and_eq(
            "coin_spent.spender",
            "juno185zc74a7w2pfxkv6d06t3kdja65tngjgg6223x",
        )
        .and_lt("tx.height", 10_187_764);

    let query2 = Query::default()
        .and_eq(
            "coin_received.receiver",
            "juno185zc74a7w2pfxkv6d06t3kdja65tngjgg6223x",
        )
        .and_lt("tx.height", 10_187_764);

    let r_task = c.tx_search(query, false, 1, 12, tendermint_rpc::Order::Descending);
    let r2_taks = c.tx_search(query2, false, 1, 15, Order::Descending);

    let (r, r2) = tokio::try_join!(r_task, r2_taks).unwrap();

    println!("{:?}", r);
}
main.rs(working)
use tendermint_rpc::{client::Client, query::Query, Order};

#[tokio::main]
async fn main() {
    let mut c = tendermint_rpc::client::HttpClient::new("https://juno-rpc.icycro.org").unwrap();
    c.set_compat_mode(tendermint_rpc::client::CompatMode::V0_37);

    let query = Query::default()
        .and_eq(
            "coin_spent.spender",
            "juno185zc74a7w2pfxkv6d06t3kdja65tngjgg6223x",
        )
        .and_lt("tx.height", 10_187_764);

    let query2 = Query::default()
        .and_eq(
            "coin_received.receiver",
            "juno185zc74a7w2pfxkv6d06t3kdja65tngjgg6223x",
        )
        .and_lt("tx.height", 10_187_764);

    let r_task = c.tx_search(query, false, 1, 12, tendermint_rpc::Order::Descending);
    let r2_taks = c.tx_search(query2, false, 1, 15, Order::Descending);

    let (r, r2) = (r_task.await.unwrap(), r2_taks.await.unwrap());

    println!("{:?}", r);
}
diff between `not working` and `working` implementation:
    // not working main
    let (r, r2) = tokio::try_join!(r_task, r2_taks).unwrap();
    // working main
    let (r, r2) = (r_task.await.unwrap(), r2_taks.await.unwrap());

Definition of "done"

concurrent calls to HttpClient should working without any side effects

@xoac xoac added the bug Something isn't working label Sep 25, 2023
@romac
Copy link
Member

romac commented Sep 26, 2023

If you add tracing-subscriber as a dependency

tracing-subscriber = { version = "0.3.17", features = ["env-filter", "fmt"] }

and install it within main()

#[tokio::main]
async fn main() {
    tracing_subscriber::fmt::init();

    // ... snip ...
}

and then run the executable with

$ RUST_LOG=tendermint_rpc=debug cargo run

then the HTTP client will output the outgoing requests it makes and their corresponding incoming responses:

     Running `/Users/romac/.cargo/target/debug/tendermint_tx_search`
2023-09-26T16:18:05.197402Z DEBUG tendermint_rpc::client::transport::http::sealed: Outgoing request: {
  "jsonrpc": "2.0",
  "id": "e7020abe-49db-42d2-af67-adfe0702ce08",
  "method": "tx_search",
  "params": {
    "query": "coin_spent.spender = 'juno185zc74a7w2pfxkv6d06t3kdja65tngjgg6223x' AND tx.height < 10187764",
    "prove": false,
    "page": "1",
    "per_page": "12",
    "order_by": "desc"
  }
}
2023-09-26T16:18:05.198328Z DEBUG tendermint_rpc::client::transport::http::sealed: Outgoing request: {
  "jsonrpc": "2.0",
  "id": "1680f0ce-8a79-481c-a44d-f656b029b3e6",
  "method": "tx_search",
  "params": {
    "query": "coin_received.receiver = 'juno185zc74a7w2pfxkv6d06t3kdja65tngjgg6223x' AND tx.height < 10187764",
    "prove": false,
    "page": "1",
    "per_page": "15",
    "order_by": "desc"
  }
}
2023-09-26T16:18:05.335952Z DEBUG tendermint_rpc::client::transport::http::sealed: Incoming response: Too Many Requests
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: serde parse error

On the second to last line, we see that the server responded with a plain text message that Too Many Requests instead of a valid JSON-RPC response.

Therefore, the issue lies with the server which appears to be doing rate limiting/throttling.

To solve this, you will have to use the machinery provided by Tokio to ensure you don't perform too many requests at once, or just not try to perform concurrent requests. There is nothing the HTTP client can do for you by default.

Hope that helps!

@romac romac closed this as completed Sep 26, 2023
@romac romac removed the bug Something isn't working label Sep 26, 2023
@romac romac closed this as not planned Won't fix, can't repro, duplicate, stale Sep 26, 2023
@romac
Copy link
Member

romac commented Sep 26, 2023

I did a quick test, and by adding a (fairly hefty) delay the node will reliably answer both queries:

    let r_task = async {
        tokio::time::sleep(tokio::time::Duration::from_millis(750)).await;
        c.tx_search(query, false, 1, 12, tendermint_rpc::Order::Descending)
            .await
    };

@xoac
Copy link
Author

xoac commented Sep 26, 2023

Thanks for founding this. I was probably tired because I did RUST_LOG=tendermint_rpc=debug before reporting this but it probably got lost with my other logs.


Overall I think this should be handled better by tendermint_rpc::HttpClient. There is possibility to bypass this error (http error) properly https://docs.rs/tendermint-rpc/0.33.2/src/tendermint_rpc/error.rs.html#50-57 (you can return one of this http::Error and hyper::Error).

On stackoverflow I found links how json-rpc should be handled when transport layer is http: https://www.simple-is-better.org/json-rpc/transport_http.html

JSON-RPC 2.0 is sent over HTTP so if HTTP return error then the body shouldn't be parsed but error should be returned.
discussion source: https://groups.google.com/g/json-rpc/c/2L_4zYIafGw/m/Oa_tEt-aAgAJ?pli=1

@romac
Copy link
Member

romac commented Sep 27, 2023

That's a good point! Let me re-open the issue then.

@romac romac reopened this Sep 27, 2023
@romac romac changed the title Can't execute concurrent requests HTTP client should not try to parse body of response with non-2XX status code Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants