Document which Error a Transport is supposed to use

[ijackson]

We're writing a binding to try to wire ureq and Arti (Tor Client) together.

Right now, if the connection across the Tor network fails, the ureq API seems to wants us to throw ureq::Error::ConnectionFailed. The result will be that the ultimate user sees almost no information about the error.

It's probably too late to improve ConnectionFailed, but maybe a different variant shoudl be provided that would allow us to transport our own error somehow (boxed, presumably). Or should we be converting everything to ureq::Error::Io? Because that contains an io::Error, we could make use of std::Io::Error's escape hatch...

[algesten]

@ijackson ConnectionFailed shouldn't happen much though.

        let transport = self
            .connector
            .connect(details, None)?
            .ok_or(Error::ConnectionFailed)?;

If there is an io::Error, that should surface on the connect() row. The ConnectionFailed is because connect return None. The chain failed to make a connection.

Can you repro with RUST_LOG=debug (or trace) and maybe get some more info?

[ijackson]

@ijackson ConnectionFailed shouldn't happen much though.

    let transport = self
        .connector
        .connect(details, None)?
        .ok_or(Error::ConnectionFailed)?;

The ConnectionFailed is because connect return None. The chain failed to make a connection.

Oh. I think you are saying that ConnectionFailed is supposed to be generated only by code in ureq? So this any ConnectFailed is (supposed to be) some kind of confguration/programming error.

That wasn't clear to us from the description. My co-contributor and I thought that this was the right variant to return from our Transport::connect.

If there is an io::Error, that should surface on the connect() row.

Connections across the Tor network can fail for a much wider variety of reasons than an underlying io::Error (which is usually just a plain OS error). arti_client has it's own Error type which is quite rich internally, and, most importantly, has a tor_error::ErrorKind.

Our ureq::Transport implementation must convert one of these errors into a ureq::Error. I think maybe an implication of what you're saying is that we should wrap it up in an io::Error, using io::Error::new?

Can you repro with RUST_LOG=debug (or trace) and maybe get some more info?

This bland error message was a prediction of the behaviour of our whole program, not seen in use. Although it seems an obvious consequence of the code we've written. I now understand that to be wrong.

[algesten]

Our ureq::Transport implementation must convert one of these errors into a ureq::Error. I think maybe an implication of what you're saying is that we should wrap it up in an io::Error, using io::Error::new?

This bland error message was a prediction of the behaviour of our whole program, not seen in use. Although it seems an obvious consequence of the code we've written. I now understand that to be wrong.

Ah, I see. I think as much as possible the strategy should be:

1. Map to io::Error if relevant
2. Map to other ureq::Error
3. Fall back on ConnectionFailed

However, I can see we might want to introduce some other error you can use to provide a more descriptive string.

[ijackson]

Ah, I see. I think as much as possible the strategy should be:

1. Map to `io::Error` if relevant

Thanks for the reply. I think i will advise my collaborator to just wrap our errors in io::Error. Dispatching based on the tor_error::ErrorKind and doing something differnet with different kinds seems more likely to be confusing than helpful.

I'm left wondering if you want to expand on your docs, for example by discouraging ureq users from creating ConnectionFailed, I think that is probably what this ticket is now about.

[algesten]

I'm left wondering if you want to expand on your docs, for example by discouraging ureq users from creating ConnectionFailed, I think that is probably what this ticket is now about.

Good idea! I will review the Error and add info on whether it's expected to be used by a bespoke Transport impl.

[algesten]

@ijackson I've made a PR #1032 that I hope would help your case. If you got time for some feedback would be great!