Don't eprintln in AsyncPgConnection

[banool]

Summary

I'm using this library from a CLI and I'd rather avoid these prints. Sadly there isn't much downstream folks can do about prints inside a library. If the library used tracing I'd switch this eprintln to use that instead, but since it doesn't, I just remove it. Let me know what you think!

Test Plan

cargo build --features postgres

[weiznich]

Thanks for opening this PR. I'm open to consider removing that eprintln in favor of better error handling. As of now the PR does not improve the error handling there, on contrary it makes things worse as there is now no indication that something went wrong. This means this PR cannot be accepted in the current state.

[banool]

Would you accept a PR to add tracing to this crate?

[banool]

The alternative would be to make a breaking change in which we return a JoinHandle from the spawned tokio task and make the user responsible for it.

[weiznich]

Would you accept a PR to add tracing to this crate?

No, that's not something I would accept as of now. At least not for handling these errors.

The alternative would be to make a breaking change in which we return a JoinHandle from the spawned tokio task and make the user responsible for it.

I'm open to consider a breaking change there as long as it does not introduce changes that are specific to this connection impl (i.e which are also reasonable for the other connection implementations). You might want to provide an overview of said changes first, to prevent working on something that will not be accepted.

[banool]

Out of curiosity, why would you not accept a PR to use tracing? At least until now, just printing the error was considered acceptable. I think tracing is quite common throughout library crates in the Rust ecosystem, whereas printing in libraries is discouraged. Do you just want to use this opportunity to add better error handling than just printing / logging?

[banool]

As for returning the JoinHandle, my plan would just to add a new field to AsyncPgConnection like connection_task_handle: JoinHandle and when a connection is established, set the handle. I'd update the TryFrom impl accordingly.

This way the establish method doesn't return anything new, so other implementations should be unaffected.

[weiznich]

Out of curiosity, why would you not accept a PR to use tracing? At least until now, just printing the error was considered acceptable. I think tracing is quite common throughout library crates in the Rust ecosystem, whereas printing in libraries is discouraged.

I have two reasons for not just using tracing for this:

• There are people that don't use tracing, so that adds an unneeded dependency for them.
• Just adding tracing for this single error message might imply to people that use tracing that there is full tracing integration for "everything" which wouldn't be the case.

As for returning the JoinHandle, my plan would just to add a new field to AsyncPgConnection like connection_task_handle: JoinHandle and when a connection is established, set the handle. I'd update the TryFrom impl accordingly.

I honestly do not see how that would improve the error handling as most user likely would ignore that field. Can you elaborate on how that would make things better, instead of just different? Also there is valid reasoning for the TryFrom impl to only need the Client, which in the end enables users to provide custom error handling on their own.

[banool]

I honestly do not see how that would improve the error handling as most user likely would ignore that field. Can you elaborate on how that would make things better, instead of just different?

As it is now, user code has no way to handle when the connection fails, all that happens is a print. If they get access to the handle they can check to see whether the task is still running, for example some kind of select_all on the handle and whatever else the user's code is doing. This is very standard with how you normally use tokio, it's expected that each thing you care about is a task and you check on their status. See the tokio tutorial: https://tokio.rs/tokio/tutorial/select. Just spawning tasks and dropping their handles is discouraged.

Yes they can still ignore it but at least the user has the ability to check if the connection is still alive, vs now where it's impossible. We could add a function on AsyncPgConnection like get_connection_handle and mark it #[must_use] if we really want to do this properly. That way users will get a lint warning if they don't deal with the handle.

[weiznich]

As it is now, user code has no way to handle when the connection fails, all that happens is a print. If they get access to the handle they can check to see whether the task is still running, for example some kind of select_all on the handle and whatever else the user's code is doing. This is very standard with how you normally use tokio, it's expected that each thing you care about is a task and you check on their status. See the tokio tutorial: https://tokio.rs/tokio/tutorial/select. Just spawning tasks and dropping their handles is discouraged.

You can do exactly that by using the provided TryFrom impl. The AsyncConnection::establish impl is only there for use cases where users do not really care about these results.

We could add a function on AsyncPgConnection like get_connection_handle and mark it #[must_use] if we really want to do this properly. That way users will get a lint warning if they don't deal with the handle.

By marking a function with #[must_use] you only get a warning if you call that function and you don't use the result. You don't get any warnings if you don't call the function.

That all written: I think better way to improve the situation would be to have some sort of error one-shot channel between the background task and the connection and select from that error channel on each async call on the connection type. So we would propagate these errors as normal errors to our users. Basically something like:

struct AsyncPgConnection {
    error_state: tokio::sync::oneshot::Receiver<diesel::result::Error>, 
    /// … other fields
}

impl AsyncConnection for AsyncPgConnection {
    async fn establish(database_url: &str) -> ConnectionResult<Self> {
        let (client, connection) = tokio_postgres::connect(database_url, tokio_postgres::NoTls)
            .await
            .map_err(ErrorHelper)?;
        let (tx, rx) = tokio::sync::oneshot::channel();
        tokio::spawn(async move {
            if let Err(e) = connection.await {
                rx.send(e.into());
                eprintln!("connection error: {e}");
            }
        });
        // normal connection setup here
        Ok(Self {error_state: rx, // other fields})
    }

    fn load<'conn, 'query, T>(&'conn mut self, source: T) -> Self::LoadFuture<'conn, 'query>
    where
        T: AsQuery + 'query,
        T::Query: QueryFragment<Self::Backend> + QueryId + 'query,
    {
        let query = source.as_query();
        let f = self.with_prepared_statement(query, |conn, stmt, binds| async move {
            let res = conn.query_raw(&stmt, binds).await.map_err(ErrorHelper)?;

            Ok(res
                .map_err(|e| diesel::result::Error::from(ErrorHelper(e)))
                .map_ok(PgRow::new)
                .boxed())
        });
         
        tokio::select! {
             error = self.error_state => return Err(error),
              res = f => res,
        }
    }
   // similar changes to other methods as well.
}

I'm happy to accept a PR that implements such a change, as long as it contains some documentation and ideally some test to show that it works.

[banool]

Alright I see the advantage to this approach since the user doesn't need to handle connection errors themselves.

This would necessitate a change to the error, the error returned by functions like load could be either the load from the inner function (QueryResult) or from the connection (tokio_postgres::error::Error). So we'd have to use thiserror, coerce one error into the other, or erase the error type entirely with something like anyhow.

Would you be happy with thiserror?

[weiznich]

Thats not needed. There is existing code for this conversion here: https://github.com/weiznich/diesel_async/blob/main/src/pg/error_helper.rs

[banool]

Awesome, in that case I'll try implement this!

[banool]

Hey hey! I've got a PR here: #121.

[weiznich]

Let's close this one in favor of #121 then