gmosx / kraken-sdk-rust

A Rust SDK for working with Kraken APIs
Apache License 2.0
23 stars 14 forks source link

Add support for timeouts #9

Closed strangelittlemonkey closed 1 year ago

strangelittlemonkey commented 1 year ago

Reqwest has built in support for timeouts, but the async libraries default to not having any timeout. When there is a network issue of some kind, surfaced more when using the library on a cron basis, it is common to see the processes hanging with the async reqwest clients never timing out. This sets a default timeout of 10s, while allowing you to pass in a value as well to override this if you desire a shorter or longer one. Also included are four unit tests, two to test that both in the futures and normal calls the default timeout is set to the default value, and two to test that in both the futures and the normal calls that when an override is specified it is respected.

gmosx commented 1 year ago

Thank you for the PR. It's a useful addition. I will merge it but probably change it a bit, for example, I don't see a need for the new constructor to have the timeout parameter, this can be handled by the builder interface.

gmosx commented 1 year ago

Where is the timeout field actually used? I.e. where is this passed to reqwest?

gmosx commented 1 year ago

Also, according to the docs, there is a default timeout of 30s: https://docs.rs/reqwest/0.8.4/reqwest/struct.ClientBuilder.html#method.timeout

strangelittlemonkey commented 1 year ago

That is for 0.8.4, which is an old version, and which predates the inclusion of the async code. After version 0.10.0, async was added and made the default, and while the blocking code was kept with timeouts, the async code was intentionally left without a default timeout.

https://docs.rs/reqwest/0.11.11/reqwest/struct.ClientBuilder.html#method.timeout

This library makes use of 0.11.11 as of the time of this comment.

https://github.com/gmosx/kraken_sdk_rust/blob/dd2747ecd39a59a004bbd6691a0b6107c9bf13f3/kraken_sdk_futures_rest/Cargo.toml#L19 https://github.com/gmosx/kraken_sdk_rust/blob/dd2747ecd39a59a004bbd6691a0b6107c9bf13f3/kraken_sdk_rest/Cargo.toml#L19

Rereading the previous comments though, you're right and I'm working on an update.

gmosx commented 1 year ago

I created this alternative PR, what do you think?

https://github.com/gmosx/kraken_sdk_rust/pull/10

strangelittlemonkey commented 1 year ago

Looks good, it's pretty much what I changed my PR to minus tests, because I hadn't changed the tests to pass and account for timeout being in a different area. It's simple enough that maybe unit tests for it were overkill, I don't know.

gmosx commented 1 year ago

OK, I merged the other one. Thank you for the suggestion.