rpc: Add HTTP timeout, default 30 seconds.

[mdyring]

Simply introduces a HTTP timeout parameter, which is passed to reqwest.

This is highly desirable a the default behaviour is "no timeout", which can result in stalls.

Closes #1379.

• Referenced an issue explaining the need for the change
• Updated all relevant documentation in docs
• Updated all code comments where relevant
• Wrote tests
• Added entry in .changelog/

[codecov-commenter]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (194d81e) 59.0% compared to head (e583973) 59.9%.

❗ Current head e583973 differs from pull request most recent head e2d3913. Consider uploading reports for the commit e2d3913 to get more accurate results

Files	Patch %	Lines
rpc/src/client/transport/http.rs	50.0%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1380     +/-   ##
=======================================
+ Coverage   59.0%   59.9%   +0.8%     
=======================================
  Files        275     275             
  Lines      27930   27529    -401     
=======================================
+ Hits       16505   16508      +3     
+ Misses     11425   11021    -404     

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

[mzabaluev]

This looks good API-wise.
Can we follow the behavior in reqwest and have "no timeout" behavior by default? This will be also the least disruptive to the current users of HttpClient.

[rllola]

Hey! We are looking forward for this to be merged because we have the issue of our request being stuck.

Anything we can do so it can be merged soon ?

[tony-iqlusion]

Note that you can use e.g. tokio::time::timeout to enforce timeouts on anything Future-based without requiring upstream support. We enforce tendermint-rpc timeouts this way in our Observatory application: https://github.com/iqlusioninc/observatory/blob/6b21705840d1c4477521b22b2d9767e23509e55d/src/client_manager.rs#L51

That said, if you do end up adding this functionality in a first-class manner, I think it would also be good if it were on-by-default.

[mdyring]

This looks good API-wise. Can we follow the behavior in reqwest and have "no timeout" behavior by default? This will be also the least disruptive to the current users of HttpClient.

I'd argue that "no timeout" behavior is unexpected and thus not a good default (even though that seems to be the default for reqwest).

As @tony-iqlusion mentioned, tokio::time::timeout is also an option for library users, so alternatively it could simply be documented that the calls might hang.

[rllola]

@tony-iqlusion Thanks for your solution.

I agree with @mdyring, the "no timeout" is unexpected and I am in favor of a default value around 30secs.

[romac]

I am totally in favor of adding a timeout parameter but would rather not enable it by default. I don't believe there exists a single good default value for such a library, since different applications will need different timeout values. Moreover, other HTTP client libraries like reqwest do not use a timeout by default. Instead, I would suggest we document that there is no timeout by default and that the call may hang indefinitely, and leave it up to users to specify a timeout if needed.

What do you think @tony-iqlusion?

[tony-iqlusion]

Personally I'm biased towards on-by-default timeouts, but having any sort of built-in timeout would be nice.

[romac]

Alright, I'll rest my case then since you guys all agree that having a timeout by default is the preferred behaviour.