Breaking change from 0.9.10 to 0.9.11 due to #240 (presumably)

[passcod]

#240 rewrote the redirect implementation away from curl. I noticed that this broke redirects in my use case.

Test code:

use isahc::{
    config::{Configurable, RedirectPolicy},
    http::request::Request,
    HttpClient,
};
use std::error::Error;

#[async_std::main]
async fn main() -> Result<(), Box<dyn Error + Send + Sync>> {
    let client = HttpClient::builder()
        .redirect_policy(RedirectPolicy::Limit(8))
        .build()
        .expect("FATAL: failed to create http client");

    let req = Request::post("http://localhost:8265/command/help").body(())?;
    let res = client.send_async(req).await?;
    dbg!(res);
    Ok(())
}

The URL issues a 302 redirect.

Output with isahc 0.9.10

[src/bin/isahc-test.rs:21] res = Response {
    status: 200,
    version: HTTP/1.1,
    headers: {
        "server": "nginx/1.18.0",
        "date": "Tue, 10 Nov 2020 08:09:31 GMT",
        "content-type": "text/plain",
        "content-length": "614",
        "last-modified": "Thu, 05 Nov 2020 10:51:25 GMT",
        "connection": "keep-alive",
        "etag": "\"5fa3d92d-266\"",
        "accept-ranges": "bytes",
    },
    body: Body(614),
}

Output with isahc 0.9.11

[src/bin/isahc-test.rs:21] res = Response {
    status: 302,
    version: HTTP/1.1,
    headers: {
        "server": "nginx/1.18.0",
        "date": "Tue, 10 Nov 2020 08:04:33 GMT",
        "content-type": "text/html",
        "content-length": "145",
        "location": "http://localhost:8265/static/help.txt",
        "connection": "keep-alive",
    },
    body: Body(145),
}

[sagebind]

Thanks for filing such a detailed reproduction of the issue! I'll take a look as soon as I can. You're right that this is almost certainly a regression from #240.

[sagebind]

Found the issue -- calling redirect_policy on an individual request builder works, but the configuration wasn't reaching the redirect interceptor when being called on the client builder. I've added a test for this to ensure this mistake isn't made again in the future. Fix is in #251.

Again, thanks for bringing this to my attention quickly. I will push a patch release tonight with the fix.

[passcod]

Awesome, thanks so much for the quick fix! ---- On Wed, 11 Nov 2020 14:20:44 +1300 [email protected] wrote ---- Found the issue -- calling redirect_policy on an individual request builder works, but the configuration wasn't reaching the redirect interceptor when being called on the client builder. I've added a test for this to ensure this mistake isn't made again in the future. Fix is in #251. Again, thanks for bringing this to my attention quickly. I will push a patch release tonight with the fix. —You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or unsubscribe.

[passcod]

Erm. I'm sorry to report that it doesn't seem to have been fixed:

[Running 'cargo run --bin isahc-test']
   Compiling isahc v0.9.12
   Compiling passcod-accord v0.0.6 (/home/code/rust/accord)
    Finished dev [unoptimized + debuginfo] target(s) in 8.95s
     Running `target/debug/isahc-test`
[src/bin/isahc-test.rs:17] res = Response {
    status: 302,
    version: HTTP/1.1,
    headers: {
        "server": "nginx/1.18.0",
        "date": "Sat, 14 Nov 2020 00:31:48 GMT",
        "content-type": "text/html",
        "content-length": "145",
        "location": "http://localhost:8265/static/help.txt",
        "connection": "keep-alive",
    },
    body: Body(145),
}
[Finished running. Exit status: 0]

Same test code.

[sagebind]

Hmm, I added a test case to ensure that it was fixed. Perhaps the test case isn't quite the same as your example. I will take another look.

[sagebind]

OK, it looks like I did fix a bug in #251 that was affecting this, but there is also a separate bug at work here. The key is that the first 302 response includes a non-zero response body. In my test it did not. If the first response that returns a redirect location returns a body, it looks like that CURLINFO_REDIRECT_URL actually returns null until you consume the entire response body. This strikes me as odd behavior on the part of curl, but after doing some research it seems like this is expected.

I will fix this by having Isahc parse the Location header itself instead of relying on curl to do it, since we don't want to have to consume the entire response body before deciding whether to redirect or not.

[sagebind]

Alright, this should now be fixed for real this time in 0.9.13! @passcod Please give it a try and don't hesitate to let me know if you have any more problems.