response: Remove error wrapper in read_next_line

[johan-bjareholt]

I'm not sure why this wrapper is here. It can result in read_next_line could return ureq::Error->std::io::Error->ureq::Error->std::io::Error where "->" refers to the source.
This makes it messy to try and find out if there was an Transport error and you want to know the Io error kind (e.g. connection reset by peer), as then you had to match both err.source() for the "ureq::Error->std::io::Error" case as well as
err.source().source().source() for the
"ureq::Error->std::io::Error->ureq::Error->std::io::Error" case. Thus, we removed the wrapper between the std::io::Error's and bundled them together into a single std::io::Error. This will make read_next_line() return an Error which always looks like this: ureq::Error -> std::io::Error.

[algesten]

The wrapper is there exactly for ).src(e); this reason right?

To preserve the stack trace of the original error.

[johan-bjareholt]

Hm, okay so this is a tradeoff between having a full stack trace and easy to parse error kinds.

In our case, we calculate statistics of different types of network errors and do that based on the std::io::ErrorKind. Since we use ureq on essentially an IoT device, the network conditions can vary a lot depending on the environment so those stats can be crucial to understand why a certain device has issues uploading. We could "un-nest" these errors ourselves on the application side, but that code would be quite dirty.

[algesten]

Can you unnest better on the ureq side?

[johan-bjareholt]

Can you unnest better on the ureq side?

Not really, same issue.

So I guess my options are:

1. Don't nest it from the start (this patch)
   • drawback: stacktrace is not preserved
2. Unnest it in my application
   • drawback: Ugly unnesting code
3. Unnest it in ureq with a helper function?
   • drawback: Still ugly unnesting code
   • Not sure if this would make sense

To give a concrete example, our current solution with this patch looks like this:

let mut status = 0
match response {
    Ok(response) => status = response.status(),
    Err(ureq::Error::Status(code, response)) => status = code,
    Err(ureq::Error::Transport(err)) => {
        // We have some custom status codes for a subset of network errors.
        // If there's no custom error code, we set status to 0.
        match err.kind() {
            ureq::ErrorKind::Dns => status = 1,
            ureq::ErrorKind::ConnectionFailed => status = 2,
            ureq::ErrorKind::Io => {
                if let Some(source) = err.source() {
                    if let Some(io_err) = source.downcast_ref::<std::io::Error>() {
                        match io_err.kind() {
                            std::io::ErrorKind::ConnectionReset => status = 3,
                            std::io::ErrorKind::BrokenPipe => status = 4,
                            std::io::ErrorKind::TimedOut => status = 5,
                            _ => (),
                        }
                    }
                }
            }
            _ => (),
        }
    }
}
log_statistic_http_upload_status(status);

Without this patch, we would have to do something messy like this

let mut status = 0
match response {
    Ok(response) => status = response.status(),
    Err(ureq::Error::Status(code, response)) => status = code,
    Err(ureq::Error::Transport(err)) => {
        // We have some custom status codes for a subset of network errors.
        // If there's no custom error code, we set status to 0.
        match err.kind() {
            ureq::ErrorKind::Dns => status = 1,
            ureq::ErrorKind::ConnectionFailed => status = 2,
            ureq::ErrorKind::Io => {
                if let Some(source) = err.source() {
                    if let Some(io_err) = source.downcast_ref::<std::io::Error>() {
                         if let Some(source) = err.source() {
                            if let Some(io_err) = source.downcast_ref::<ureq::Error>() {
                                 if let Some(source) = err.source() {
                                    if let Some(io_err) = source.downcast_ref::<std::io::Error>() {
                                        match io_err.kind() {
                                            std::io::ErrorKind::ConnectionReset => status = 3,
                                            std::io::ErrorKind::BrokenPipe => status = 4,
                                            std::io::ErrorKind::TimedOut => status = 5,
                                            _ => (),
                                        }
                                    }
                                }
                            }
                        } else {
                            match io_err.kind() {
                                std::io::ErrorKind::ConnectionReset => status = 3,
                                std::io::ErrorKind::BrokenPipe => status = 4,
                                std::io::ErrorKind::TimedOut => status = 5,
                                _ => (),
                            }
                        }
                    }
                }
            }
            _ => (),
        }
    }
}
log_statistic_http_upload_status(status);

[algesten]

You can't use a for-loop or recursion to go down?

[johan-bjareholt]

Good point, recursion works fine. Now I'm annoyed at myself for not coming up with that solution myself 😅

I guess I can close this PR then, I assume that a helper function that does the recursion to find the root error is not interesting enough to upstream as a helper function?

[johan-bjareholt]

This is how I ended up implementing it in the application

fn error_get_root_source<'a>(err: &'a (dyn Error + 'static)) -> &'a (dyn Error + 'static) {
    if let Some(err) = err.source() {
        error_get_root_source(err) as &(dyn Error + 'static)
    } else {
        err as &(dyn Error + 'static)
    }
}

Thanks for the help!