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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error sink not called unless debugger is attached with breakpoints #151

Closed
nyurik opened this issue Feb 3, 2023 · 2 comments
Closed

Error sink not called unless debugger is attached with breakpoints #151

nyurik opened this issue Feb 3, 2023 · 2 comments

Comments

@nyurik
Copy link
Contributor

nyurik commented Feb 3, 2023

This code reproduces the issue fairly reliably on my Linux box with Rust 1.67. For debugging I used IntelliJ, but I don't know if the same behavior is on VS code.

Error Sink is not called when PostgreSQL reports a connection error on pool.get(), unless I run this code in a debugger with two breakpoints set. Instead of the actual PG error, it reports a timeout error without calling the error sink.

Make sure to run postgres in a docker container (see message in code). The two commented-out test lines work fine.

// [dependencies]
// bb8 = "0.8"
// bb8-postgres = "0.8"
// env_logger = "0.10"
// log = "0.4"
// postgres = "0.19"
// tokio = { version = "1", features = ["full"] }

use std::str::FromStr;

type PgConnManager = bb8_postgres::PostgresConnectionManager<postgres::NoTls>;
type PgPool = bb8::Pool<PgConnManager>;
type PgConnError = <PgConnManager as bb8::ManageConnection>::Error;

#[derive(Debug, Clone, Copy)]
struct PgErrorSink;

impl bb8::ErrorSink<PgConnError> for PgErrorSink {
    fn sink(&self, e: PgConnError) {
        println!("ErrorSink pg error: {e}");
    }

    fn boxed_clone(&self) -> Box<dyn bb8::ErrorSink<PgConnError>> {
        println!("Cloning ErrorSink");
        Box::new(*self)
    }
}

#[tokio::main]
async fn main() -> Result<(), bb8::RunError<postgres::Error>> {
    // Allow this type of invocation:   RUST_LOG=TRACE cargo run
    env_logger::Builder::from_env(env_logger::Env::default()).init();
    println!("Make sure this is running:");
    println!("  docker run --rm -it -e POSTGRES_PASSWORD=postgres -p 5401:5432 postgres");

    // test("postgres://postgres:postgres@127.0.0.1:5401/postgres").await?;
    // test("postgres://postgres:postgres@127.0.0.1:5401/postgres?sslmode=disable").await?;
    test("postgres://postgres:postgres@127.0.0.1:5401/postgres?sslmode=require").await?;
    Ok(())
}

async fn test(conn_str: &str) -> Result<(), bb8::RunError<postgres::Error>> {
    println!("\nConnecting to {conn_str}");
    let pg_cfg = bb8_postgres::tokio_postgres::config::Config::from_str(conn_str)?;

    let pool = PgPool::builder()
        .max_size(1)
        .connection_timeout(std::time::Duration::from_secs(5))
        .error_sink(Box::new(PgErrorSink))
        .build(PgConnManager::new(pg_cfg, postgres::NoTls))
        .await?;

    let query = "SELECT version();";
    let val = pool.get().await?.query_one(query, &[]).await?;
    println!("Version: {:?}", val.get::<_, String>(0));
    Ok(())
}

Running without debugger

The above code prints Error: TimedOut

Running with debugger

I was able to see the error sink error if I set two breakpoints in the debugger (I used IntelliJ) -- note that both breakpoints are required for this to work.

Run the code and keep hitting F9 (Resume), and it prints this:

ErrorSink pg error: error performing TLS handshake: server does not support TLS
Error: TimedOut
@djc
Copy link
Owner

djc commented Feb 3, 2023

Sorry about that -- sounds like some kind of odd race condition is making it timing-dependent? I'm afraid I won't have much time to investigate, but happy to support you if you're able to dig in more.

I think this might be the same as #141?

nyurik added a commit to maplibre/martin that referenced this issue Feb 6, 2023
This is a partial fix for #496

* BREAKING: Now Martin behaves the same way as `psql` -- by default, if
SSL is available on the server, it will be used, even though it will not
verify that the server has a valid SSL certificate
* Martin now understands `PGSSLCERT`, `PGSSLKEY`, and `PGSSLROOTCERT`
env vars (and corresponding config keys) - same as psql.
* Martin can now process `?sslmode=verify-ca` and `verify-full` (just
like psql). The verify modes require root and/or client cert & key.
* remove `danger_accept_invalid_certs` -- turns out that behavior is
expected by default unless ssl mode is set to verify - which upstream
lib [does not
support](sfackler/rust-postgres#768) - PR
[submitted](sfackler/rust-postgres#988).
* added connection_timeout_ms option for postgres and set it to 5
seconds by default. This way it will fail out earlier.
* added error reporting to bb8 - but it is currently [broken
upstream](djc/bb8#151) - not sure we can fix
it easily, so may need to switch to deadpool later.
* added docker-based TLS test (horray!) - wasn't trivial at all, despite
ending up fairly simple.
@couchand
Copy link
Contributor

Yes this looks very much the same as #141. It looks like the debugger's breakpoints allow the error to be dispatched to the sink before the main future completes, but this is unlikely to happen in the general case for the same reasons I describe there.

@djc djc closed this as not planned Won't fix, can't repro, duplicate, stale Mar 21, 2023
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

3 participants