[doc] Encourage the user to handle connection properly

[awnion]

The current version on the main page of the documentation doesn't encourage users to handle the connection coroutine. One may (and I already have multiple pieces of evidence) use this example in the real app with a long-running server where connection panic doesn't affect the other part of the app leading to the inconsistent state.

Even simple panic would be better in this case since you explicitly rely on the outside mechanisms to deal with it (e.g. docker or any other runtime/supervisor).

[sfackler]

What is the "irrecoverable state"?

[awnion]

What is the "irrecoverable state"?

Hm... I just reread and realized that it doesn't make sense in this particular example + it will be blocked on connection_handler.await...

So I need some advice on what I want. I'll try to make it more clear:

In the example if the connection panics right before the client.query call, the client query will return an error which is expected behavior, right? I just don't like the fact that the app will fail on an attempt to use the connection which is already gone, and not at the moment when the connection breaks. Of course for the example, it's a huge overstretch :)

Yet if we put that code into the real app -- it will cause the problem with the "irrecoverable state". E.g.:

    tokio::spawn(async move {
        if let Err(e) = connection.await {
            eprintln!("connection error: {}", e);
        }
    });

    // connection panics at some point
    loop {
        // worker
    }

Should this example panic? In my opinion yes (because the worker can't restore the connection back, since the context is lost, am I right?)

So should I provide an extra example with a "long-running" task? e.g.

#[tokio::main]
async fn main() -> Result<(), Error> {
    // Connect to the database.
    let (client, connection) =
        tokio_postgres::connect("host=localhost user=postgres", NoTls).await?;

    let connection_handler = tokio::spawn(async move {
        if let Err(e) = connection.await {
            eprintln!("connection error: {}", e);
        }
    });

    let server_handler = tokio::spawn(async move {
        // web server or any other long-running task
        let listener = TcpListener::bind("127.0.0.1:8080")
            .await
            .expect("todo: handle");
        let (_, _) = listener.accept().await.expect("todo: handle");
        let _ = client.query("SELECT 1", &[]).await.expect("todo: handle");
    });

    tokio::select! {
        v = connection_handler => v.expect("todo: handle"),
        v = server_handler => v.expect("todo: handle"),
    }

    Ok(())
}

[sfackler]

Have you observed the connection panicking? That should never happen in any circumstance.

If that does happen methods on the client will return errors, the same as what would happen if the connection exited without panicking because e.g. the server disconnected. You should use a connection pool for long lived processes to ensure that you keep connections ready over time.

[awnion]

Correct me if I'm wrong but I feel like it's not intuitively a desired behavior when you have to wait for the next e.g. client.query to realize that the DB connection is closed. I would prefer to panic from the app as long as I realize that the DB connection is closed and can't be restored (e.g. the db is down).

UPD In other words

    tokio::spawn(async move { panic!("assume DB disconnect") });

    sleep(Duration::from_secs(10000000)).await;

    let _ = client.query("SELECT 1", &[]).await?;

Intuitively I want this code to close the app under 1 sec.

[sfackler]

I can't speak to what behavior you want, but the change in this PR will make the process block until the database goes down which I doubt is desired by ~anyone.

[awnion]

Oh of course I agree with that. Sorry for the confusion. I should close this one.