HEAD requests which return a content-length incorrectly Err
Firstyear opened this issue · 15 comments
Some urls will return a content-length during head requests. Surf incorrectly assumes that this means there is a body present and will error:
thread 'main' panicked at 'Should Succeed!: ResponseBodyError(None): unknown error', src/main.rs:9:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
The following reproducer can show this behaviour:
[package]
name = "surf-cl-repro"
version = "0.1.0"
edition = "2018"
[dependencies]
surf = "2.2"
url = "2"
[dependencies.async-std]
version = "1.7.0"
features = ["attributes"]use url::Url;
#[async_std::main]
async fn main() {
let client = surf::client().with(surf::middleware::Redirect::new(2));
let url = Url::parse("http://download.opensuse.org/update/tumbleweed/repodata/repomd.xml")
.expect("invalid url");
let req = surf::head(url);
client.send(req).await.expect("Should Succeed!");
}Expected Results: Surf should allow head requests to proceed even if a content-length is returned.
Can you run with RUST_BACKTRACE=1? I suspect this is an async-h1 issue.
The documentation for HTTP HEAD explicitly call out Content-Length as an example of a request which should succeed. This is definitely a bug.
For example, if a URL might produce a large download, a HEAD request could read its Content-Length header to check the filesize without actually downloading the file.
Oh. I think this is a libcurl / isahc problem, it seems familiar.
@Firstyear try using this in your Cargo.toml:
surf = { version = "2.3", default-features = false, features = ["h1-client", "encoding"] }RUST_BACKTRACE=1 for @Fishrock123 (no changes to features)
thread 'main' panicked at 'Should Succeed!: ResponseBodyError(None): unknown error', src/main.rs:9:28
stack backtrace:
0: rust_begin_unwind
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:493:5
1: core::panicking::panic_fmt
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/panicking.rs:92:14
2: core::result::unwrap_failed
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/result.rs:1355:5
3: core::result::Result<T,E>::expect
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/result.rs:997:23
4: surf_cl_repro::main::main::{{closure}}
at ./src/main.rs:9:5
5: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/future/mod.rs:80:19
6: surf_cl_repro::main::{{closure}}
at ./src/main.rs:3:1
7: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/future/mod.rs:80:19
8: <async_std::task::builder::SupportTaskLocals<F> as core::future::future::Future>::poll::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:199:17
9: async_std::task::task_locals_wrapper::TaskLocalsWrapper::set_current::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/task_locals_wrapper.rs:60:13
10: std::thread::local::LocalKey<T>::try_with
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:376:16
11: std::thread::local::LocalKey<T>::with
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:352:9
12: async_std::task::task_locals_wrapper::TaskLocalsWrapper::set_current
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/task_locals_wrapper.rs:55:9
13: <async_std::task::builder::SupportTaskLocals<F> as core::future::future::Future>::poll
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:197:13
14: <futures_lite::future::Or<F1,F2> as core::future::future::Future>::poll
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-lite-1.12.0/src/future.rs:526:33
15: async_executor::Executor::run::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-executor-1.4.1/src/lib.rs:242:9
16: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/future/mod.rs:80:19
17: async_executor::LocalExecutor::run::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-executor-1.4.1/src/lib.rs:447:9
18: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/future/mod.rs:80:19
19: async_io::driver::block_on
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-io-1.6.0/src/driver.rs:142:33
20: async_global_executor::reactor::block_on::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-global-executor-2.0.2/src/reactor.rs:3:18
21: async_global_executor::reactor::block_on
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-global-executor-2.0.2/src/reactor.rs:12:5
22: async_global_executor::executor::block_on::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-global-executor-2.0.2/src/executor.rs:26:36
23: std::thread::local::LocalKey<T>::try_with
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:376:16
24: std::thread::local::LocalKey<T>::with
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:352:9
25: async_global_executor::executor::block_on
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-global-executor-2.0.2/src/executor.rs:26:5
26: async_std::task::builder::Builder::blocking::{{closure}}::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:171:25
27: async_std::task::task_locals_wrapper::TaskLocalsWrapper::set_current::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/task_locals_wrapper.rs:60:13
28: std::thread::local::LocalKey<T>::try_with
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:376:16
29: std::thread::local::LocalKey<T>::with
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:352:9
30: async_std::task::task_locals_wrapper::TaskLocalsWrapper::set_current
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/task_locals_wrapper.rs:55:9
31: async_std::task::builder::Builder::blocking::{{closure}}
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:168:17
32: std::thread::local::LocalKey<T>::try_with
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:376:16
33: std::thread::local::LocalKey<T>::with
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:352:9
34: async_std::task::builder::Builder::blocking
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:161:9
35: async_std::task::block_on::block_on
at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/block_on.rs:33:5
36: surf_cl_repro::main
at ./src/main.rs:3:1
37: core::ops::function::FnOnce::call_once
at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
With surf = { version = "2.3", default-features = false, features = ["h1-client", "encoding"] } this succeeds so I think you are correct about it being a isahc issue :)
We may want to report that to the isahc repo, but note that we run 0.9 instead of 1.x
Is there a major set of changes to move to 1.x first? And did you want me to report that issue or can we proxy it over somehow?
@Firstyear We probably need to introduce versioned feature flags ...
Please report this to Isahc, yeah
Actually this looks the same as #218 perhaps?
I dug into this example program with LLDB and figured out the cause; it is kind of a Surf bug, and kind of an Isahc bug. Let's start with the former.
Surf is constructing a request for Isahc to send, and gives it a request body to upload of length 0. Note that this is not the same as the absence of a request body! The way I interpret the spec (RFC 7230, Section 3.3), there is a clear semantic difference between a zero-length payload and the absence of a payload. In particular:
The presence of a message body in a request is signaled by a
Content-LengthorTransfer-Encodingheader field. Request message framing is independent of method semantics, even if the method does not define any use for a message body.
Or maybe even more clear:
[...] All other responses do include a message body, although the body might be of zero length.
This implies that a message with a Content-Length: 0 request header is semantically different than a request with no such header and no body. Isahc is following this distinction here in that isahc::Body::from_reader_sized(body, 0) is not the same as isahc::Body::empty()! Surf is always doing the former, even for HEAD requests, here: https://github.com/http-rs/http-client/blob/3c0ed9a2f350749f48f579867139dd9e3afdf182/src/isahc.rs#L53-L56.
Maybe Isahc should have represented bodies as Option<Body> to be more semantically clear, but that ship has sailed for now.
Now this is a relatively minor bug, but unfortunately it triggers an Isahc bug described in sagebind/isahc#342, in which Isahc fails trying to read a response body when the server sends a response with a Content-Length header to such a HEAD request containing a request body. For now I'll work on fixing the Isahc bug. If you'd really like to avoid upgrading to 1.x for right now I can backport the bugfix to the 0.9.x series too.
The issue you are describing seems like it's an api shortcoming of how isahc works with the http crate... The problem stems from the interactions of this api: https://docs.rs/http/0.2.4/http/request/struct.Builder.html#method.body
I think I can work around this with some extra code though, for better or worse...
Could someone check if this patch fixes the issue? http-rs/http-client#97 (Edit: original patch was wrong, should be good now.)
Regarding isahc 0.9 vs 1.x - we ("we" as in who ever did this at the time, who was not really me (i.e. pre 2.x)) messed up and made feature flags on multiple clients which do not pin to their major versions...
curl-client is a bad flag name anyways, and should have been something like curl-isahc09-client, then we could have easily had an curl-isahc1-client, etc. the same absurd problem exists for hyper / tokio, fwiw.
Tested with:
[patch.crates-io]
http-client = { git = "https://github.com/Fishrock123/http-client.git", branch = "fix-isahc-head-zero-size" }
Can confirm that it works now :)
Thank you!
There may be another way to trigger this, as I've started to see other calls have this issue now.
[package]
name = "surf-cl-repro"
version = "0.1.0"
edition = "2018"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dependencies]
surf = "2.3"
url = "2"
tracing-subscriber = "0.2"
[dependencies.async-std]
version = "1.7.0"
features = ["attributes"]
[patch.crates-io]
http-client = { git = "https://github.com/Fishrock123/http-client.git", branch = "fix-isahc-head-zero-size" }
use url::Url;
use tracing_subscriber;
#[async_std::main]
async fn main() {
tracing_subscriber::fmt::init();
let client = surf::client().with(surf::middleware::Redirect::new(3));
let url = Url::parse("http://download.opensuse.org/tumbleweed/iso/openSUSE-Tumbleweed-NET-x86_64-Current.iso")
.expect("invalid url");
let req = surf::head(url);
let res = client.send(req).await.expect("Should Succeed!");
println!("{:?}", res);
}
Perhaps the fixed branch is incomplete?
@Fishrock123 I think I've solved it. Looking at http-client/src/isahc.rs I think you need to match on Some(0) | None => because in some cases it does a Some(0) which would still triger the other path.