Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't duplicate error messages #2147

Open
konstin opened this issue Feb 29, 2024 · 0 comments
Open

Don't duplicate error messages #2147

konstin opened this issue Feb 29, 2024 · 0 comments

Comments

@konstin
Copy link

konstin commented Feb 29, 2024

When showing the error chain, reqwest duplicates all the error messages:

For a timeout:

馃挜 my-app failed
  Caused by: Network request failed
  Caused by: error sending request for url (https://httpbin.org/): operation timed out
  Caused by: operation timed out

For no internet connection:

馃挜 my-app failed
  Caused by: Network request failed
  Caused by: error sending request for url (https://httpbin.org/): error trying to connect: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: error trying to connect: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: failed to lookup address information: Temporary failure in name resolution

The inlining that causes the duplication happens here:

reqwest/src/error.rs

Lines 209 to 211 in 68547a0

if let Some(e) = &self.inner.source {
write!(f, ": {e}")?;
}

This is unlike what most of our other dependencies do and makes error quite verbose. Would it be possible to not inline the source field and leave that to the user?

For reproduction, the example program:

use std::time::Duration;
use reqwest::ClientBuilder;
use thiserror::Error;

#[derive(Debug, Error)]
enum MyError {
    #[error("Network request failed")]
    NetworkError(#[from] reqwest::Error)
}

async fn run() -> Result<(), MyError> {
    let client = ClientBuilder::new().timeout(Duration::from_millis(1)).build()?;
    client.get("https://httpbin.org").send().await?.text().await?;
    Ok(())
}

#[tokio::main]
async fn main() {
    if let Err(err) = run().await {
        eprintln!("馃挜 my-app failed");

        let mut last_error: Option<&(dyn std::error::Error + 'static)> = Some(&err);
        while let Some(err) = last_error {
            eprintln!("  Caused by: {err}");
            last_error = err.source();
        }
    }
}
reqwest = {  git = "https://github.com/seanmonstar/reqwest", rev = "68547a0af66222960eea6d649bf94497c11df246" }
thiserror = "1.0.57"
tokio = { version = "1.36.0", features = ["macros", "rt-multi-thread"] }
konstin added a commit to astral-sh/uv that referenced this issue Mar 1, 2024
Error for `uv pip compile scripts/requirements/jupyter.in` without internet:

**Before**

```
error: error sending request for url (https://pypi.org/simple/jupyter/): error trying to connect: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: error trying to connect: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: failed to lookup address information: Temporary failure in name resolution
```

**After**

```
error: error sending request for url (https://pypi.org/simple/jupyter/): error trying to connect: dns error: No such host is known. (os error 11001)
  Caused by: Could not connect, are you offline?
```

The implementation checks for "dne error" stringly as hyper errors are opaque. The danger is that this breaks with a hyper update. We still get the complete error trace since reqwest eagerly inlines errors (seanmonstar/reqwest#2147).

No test since i wouldn't know how to simulate this in cargo test.

Fixes #1971
konstin added a commit to astral-sh/uv that referenced this issue Mar 4, 2024
Error for `uv pip compile scripts/requirements/jupyter.in` without
internet:

**Before**

```
error: error sending request for url (https://pypi.org/simple/jupyter/): error trying to connect: dns error: failed to lookup address information: No such host is known. (os error 11001)
  Caused by: error trying to connect: dns error: failed to lookup address information: No such host is known. (os error 11001)
  Caused by: dns error: failed to lookup address information: No such host is known. (os error 11001)
  Caused by: failed to lookup address information:  No such host is known. (os error 11001)
```

**After**

```
error: Could not connect, are you offline?
  Caused by: error sending request for url (https://pypi.org/simple/django/): error trying to connect: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: error trying to connect: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: dns error: failed to lookup address information: Temporary failure in name resolution
  Caused by: failed to lookup address information: Temporary failure in name resolution
```

On linux, it would be "Temporary failure in name resolution" instead of
"No such host is known. (os error 11001)".

The implementation checks for "dne error" stringly as hyper errors are
opaque. The danger is that this breaks with a hyper update. We still get
the complete error trace since reqwest eagerly inlines errors
(seanmonstar/reqwest#2147).

No test since i wouldn't know how to simulate this in cargo test.

Fixes #1971
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant