chore: change internal component client http to handle errors in fail…

[uriel-starkware]

…ure and propogate it

---

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.08%. Comparing base (a29c6a0) to head (36d89b5).
Report is 1 commits behind head on main.

Files	Patch %	Lines
crates/mempool_infra/src/component_client.rs	76.92%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
- Coverage   83.09%   83.08%   -0.02%     
==========================================
  Files          31       31              
  Lines        1532     1537       +5     
  Branches     1532     1537       +5     
==========================================
+ Hits         1273     1277       +4     
  Misses        196      196              
- Partials       63       64       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 83 at r1 (raw file):

                serialize(&component_request).expect("Request serialization should succeed"),
            ))
            .expect("Request builidng should succeed");

Not part of this pr, but there's a typo here: builidng -> building (swapped "d" with "i")

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

            .await
            .map_err(|_e| ClientError::ConnectionFailure)?;
        let body_bytes = to_bytes(http_response.into_body()).await.expect("Body should exist");

Can we use aggregate here instead?

Code quote:

.into_body()

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

            .await
            .map_err(|_e| ClientError::ConnectionFailure)?;
        let body_bytes = to_bytes(http_response.into_body()).await.expect("Body should exist");

Can we use aggregate instead?

Code quote:

to_bytes

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 89 at r1 (raw file):

            .request(http_request)
            .await
            .map_err(|_e| ClientError::ConnectionFailure)?;

This represent a general http error, correct? If so, please:

1. Please rename to CommunicationFailure, this name better captures the "we've-successfully-connected-but-got-another-error" potential case.
2. Change the enum to include a presentation of the error (http status code?)

Code quote:

ClientError::ConnectionFailure

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

            .await
            .map_err(|_e| ClientError::ConnectionFailure)?;
        let body_bytes = to_bytes(http_response.into_body()).await.expect("Body should exist");

When can this fail?

Code quote:

.expect("Body should exist")

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 105 at r1 (raw file):

        Self { uri: self.uri.clone(), _req: PhantomData, _res: PhantomData }
    }
}

Did you get a compilation error that lead to this code being here?

Code quote:

// Can't derive because derive forces the generics to also be `Clone`, which we prefer not to do
// since it'll require transactions to be cloneable.
impl<Request, Response> Clone for ComponentClientHttp<Request, Response>
where
    Request: Serialize,
    Response: for<'a> Deserialize<'a>,
{
    fn clone(&self) -> Self {
        Self { uri: self.uri.clone(), _req: PhantomData, _res: PhantomData }
    }
}

---

crates/mempool_infra/src/component_client.rs line 109 at r1 (raw file):

#[derive(Debug, Error, PartialEq)]
pub enum ClientError {
    #[error("Could not conenct to server")]

typo

Code quote:

conenct

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 96 at r1 (raw file):

// Can't derive because derive forces the generics to also be `Clone`, which we prefer not to do
// since it'll require transactions to be cloneable.

Please replace "transactions" with "the generic Requests and Responses".

Code quote:

transactions

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_http_test.rs line 74 at r1 (raw file):

async fn verify_response(
    a_client: &ComponentClientHttp<ComponentARequest, ComponentAResponse>,

Please change to ownership (i.e., remove ref), and invoke with a clone of the client.

Code quote:

&ComponentClientHttp<ComponentARequest, ComponentAResponse>,

---

crates/mempool_infra/tests/component_server_client_http_test.rs line 81 at r1 (raw file):

async fn verify_error(
    a_client: &ComponentClientHttp<ComponentARequest, ComponentAResponse>,

Ditto

Code quote:

&ComponentClientHttp<ComponentARequest, ComponentAResponse>,

---

crates/mempool_infra/tests/component_server_client_http_test.rs line 101 at r1 (raw file):

        ComponentClientHttp::<ComponentBRequest, ComponentBResponse>::new(local_ip, b_port);

    verify_error(&a_client, ClientError::ConnectionFailure).await;

Please create a separate test for the error scenario. Please see if there is functionality that is required in both tests, and decide if it makes sense to wrap it in a function.

Code quote:

 verify_error(&a_client, ClientError::ConnectionFailure).await;

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_http_test.rs line 101 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please create a separate test for the error scenario. Please see if there is functionality that is required in both tests, and decide if it makes sense to wrap it in a function.

Also, after extending the enum to include the error, please consider adding various tests for the different expected errors.

[uriel-starkware]

Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_infra/src/component_client.rs line 83 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Not part of this pr, but there's a typo here: builidng -> building (swapped "d" with "i")

Done.

---

crates/mempool_infra/src/component_client.rs line 89 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This represent a general http error, correct? If so, please:

1. Please rename to CommunicationFailure, this name better captures the "we've-successfully-connected-but-got-another-error" potential case.
2. Change the enum to include a presentation of the error (http status code?)

regarding 2. I propose to do it in another PR that will expand the various options of failures. This new PR will include a dedicated test

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

When can this fail?

Hypothetically or in a real scenario? As far as we control both ends of communications, there should be a body because we send it.
There could be maybe a

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Can we use aggregate instead?

I need an explanation. what is aggregate in this context? I found a function aggregate that returns an "impl buf", I pass to the next function &[u8]

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Can we use aggregate here instead?

I need an explanation. what is aggregate in this context? I found a function aggregate that returns an "impl buf", I pass to the next function &[u8]

---

crates/mempool_infra/src/component_client.rs line 96 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please replace "transactions" with "the generic Requests and Responses".

Done.

---

crates/mempool_infra/src/component_client.rs line 105 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Did you get a compilation error that lead to this code being here?

I made changes to the test and found out that I don't have the ability to clone the client and I needed it be clonable to save unnecessary new creations of a_client

---

crates/mempool_infra/tests/component_server_client_http_test.rs line 74 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change to ownership (i.e., remove ref), and invoke with a clone of the client.

Done.

---

crates/mempool_infra/tests/component_server_client_http_test.rs line 81 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Ditto

Done.

---

crates/mempool_infra/tests/component_server_client_http_test.rs line 101 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Also, after extending the enum to include the error, please consider adding various tests for the different expected errors.

Note my proposal for a new pr to do this, in the meantime, we will not separate into two tests

[uriel-starkware]

Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, uriel-starkware wrote…

Hypothetically or in a real scenario? As far as we control both ends of communications, there should be a body because we send it.
There could be maybe a

... corruption to data or unexpected response with no body, I think I will address it with the followup PR

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 96 at r1 (raw file):

Previously, uriel-starkware wrote…

Done.

Apologies for the hassle, another slight change: "the generic Requests and Responses" -> "the generic Request and Response types"

---

crates/mempool_infra/src/component_client.rs line 105 at r1 (raw file):

Previously, uriel-starkware wrote…

I made changes to the test and found out that I don't have the ability to clone the client and I needed it be clonable to save unnecessary new creations of a_client

Ack.

---

crates/mempool_infra/tests/component_server_client_http_test.rs line 101 at r1 (raw file):

Previously, uriel-starkware wrote…

Note my proposal for a new pr to do this, in the meantime, we will not separate into two tests

Ack.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 89 at r1 (raw file):

Previously, uriel-starkware wrote…

regarding 2. I propose to do it in another PR that will expand the various options of failures. This new PR will include a dedicated test

Ack. Next time please add a todo in the code to indicate this is your intention 🙏

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, uriel-starkware wrote…

... corruption to data or unexpected response with no body, I think I will address it with the followup PR

I'm worried that this will result in a panic and the process will crash, without reporting the issue. Please change to propagate this error. IMO this is in the scope of this pr.

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, uriel-starkware wrote…

I need an explanation. what is aggregate in this context? I found a function aggregate that returns an "impl buf", I pass to the next function &[u8]

The to_bytes doc suggests that it copies all data into a single continous buffer, and suggests to use aggregate instead:
https://docs.rs/hyper/0.14.29/hyper/body/fn.to_bytes.html

AFAIU aggregate returns a impl Buf object:
https://docs.rs/hyper/0.14.29/hyper/body/fn.aggregate.html

The buf doc suggests an optimizied way to receive the required bytes
https://docs.rs/hyper/0.14.29/hyper/body/trait.Buf.html#method.copy_to_bytes

I'm not sure which of these is better, and if so, what's the performance gain, hence my question from the previous message.

Is there a simple way to measure which one is better?

[uriel-starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_infra/src/component_client.rs line 89 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Ack. Next time please add a todo in the code to indicate this is your intention 🙏

I will add now, thanks

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

The to_bytes doc suggests that it copies all data into a single continous buffer, and suggests to use aggregate instead:
https://docs.rs/hyper/0.14.29/hyper/body/fn.to_bytes.html

AFAIU aggregate returns a impl Buf object:
https://docs.rs/hyper/0.14.29/hyper/body/fn.aggregate.html

The buf doc suggests an optimizied way to receive the required bytes
https://docs.rs/hyper/0.14.29/hyper/body/trait.Buf.html#method.copy_to_bytes

I'm not sure which of these is better, and if so, what's the performance gain, hence my question from the previous message.

Is there a simple way to measure which one is better?

My 2 cents in this case is that to_bytes is simpler, and it is enough for now.

In more detail: aggregate works if I don't need a contiguous buffer which is not the case (I deserialize it into another type). If I will use aggregate, I will use copy_to_bytes anyway and it will use the impl for hyper::body, which I don't control. The only thing I can achieve from using aggregate and copy_to_bytes is to tell it how many bytes I want to copy, which again, I don't want to manage.
Maybe in the future, if we find that this is a bottleneck, we should consider benchmarking it and testing which one is better and if needed, delve into the fine details

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I'm worried that this will result in a panic and the process will crash, without reporting the issue. Please change to propagate this error. IMO this is in the scope of this pr.

Done.

---

crates/mempool_infra/src/component_client.rs line 96 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Apologies for the hassle, another slight change: "the generic Requests and Responses" -> "the generic Request and Response types"

Done.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 90 at r1 (raw file):

Previously, uriel-starkware wrote…

My 2 cents in this case is that to_bytes is simpler, and it is enough for now.

In more detail: aggregate works if I don't need a contiguous buffer which is not the case (I deserialize it into another type). If I will use aggregate, I will use copy_to_bytes anyway and it will use the impl for hyper::body, which I don't control. The only thing I can achieve from using aggregate and copy_to_bytes is to tell it how many bytes I want to copy, which again, I don't want to manage.
Maybe in the future, if we find that this is a bottleneck, we should consider benchmarking it and testing which one is better and if needed, delve into the fine details

Ack.

---

crates/mempool_infra/src/component_client.rs line 114 at r3 (raw file):

    #[error("Could not connect to server")]
    CommunicationFailure,
    #[error("Could not extract body from HTTP response: {0}")]

Could not parse the response : {0}"

Code quote:

Could not extract body from HTTP response: {0}"

---

crates/mempool_infra/src/component_client.rs line 115 at r3 (raw file):

    CommunicationFailure,
    #[error("Could not extract body from HTTP response: {0}")]
    BodyExtractionFailure(String),

Please change to "ResponseParsingFailure".

Code quote:

BodyExtractionFailure

[uriel-starkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_infra/src/component_client.rs line 114 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Could not parse the response : {0}"

Done.

---

crates/mempool_infra/src/component_client.rs line 115 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change to "ResponseParsingFailure".

Done.

[Itay-Tsabary-Starkware]

Reviewed 1 of 2 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @uriel-starkware)