bitreq: use Duration for Request::with_timeout#642
Conversation
luisschwab
requested review from
TheBlueMatt,
apoelstra,
jamillambert,
oleonardolima,
tcharding and
tnull
as code owners
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Generally makes sense to me.
| let secs = self.timeout.as_secs(); | ||
| if secs == 0 && self.timeout > Duration::from_secs(0) { | ||
| 1 | ||
| Duration::from_secs(1) |
There was a problem hiding this comment.
Codex:
- [P1] /home/tnull/workspace/corepc-pr-642/jsonrpc/Cargo.toml:41 still allows bitreq = "0.3.5", but jsonrpc now calls the new with_timeout(Duration) API. Published or locked downstream builds can still resolve an older bitreq with with_timeout(u64) and fail to compile when bitreq_http or
bitreq_http_async is enabled. Bump the dependency to the first bitreq release containing this breaking API, likely alongside a bitreq version bump.
There was a problem hiding this comment.
Should we bump bitreq to v0.3.8 (or maybe to v0.4.0, since this is a breaking change, although minimal) in this same PR?
Bumping bitreq to v0.3.7 on jsonrpc would be of no effect, since this change is still unreleased.
There was a problem hiding this comment.
Easier to keep track of that way imo, but up to you.
There was a problem hiding this comment.
We don't need to release this immediately, right? Why don't we just bump the version when we go to release?
There was a problem hiding this comment.
OP is not correct IIUC, we have a path section in the dep.
luisschwab
force-pushed
the
bitreq/use-duration-for-timeout
branch
from
dfdaf26 to
c5e4d78
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
luisschwab
force-pushed
the
bitreq/use-duration-for-timeout
branch
from
c5e4d78 to
6734bc8
Compare
|
@tcharding fixed |
|
Will wait for one of the others to ack before merging. Thanks @luisschwab. |
satsfy
left a comment
satsfy
left a comment
There was a problem hiding this comment.
tACK 6734bc8
Since my last review, the CI has been fixed, and no functional changes have been made. Tested the new duration locally.
5 participants
Closes #641
Uses
DurationforReqwest::with_timeout, sinceu64leaves a bit of room for doubt.