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

Query hangs when max_size is greater than the current amount of idle connections #113

Open
SeanOMik opened this issue Nov 9, 2021 · 2 comments

Comments

@SeanOMik
Copy link

SeanOMik commented Nov 9, 2021

I made a Manager for scylla and it worked great when I had the max_size set to the same amount as min_idle. Here's my implementation:

use bb8::ManageConnection;

use scylla::{Session, SessionBuilder, SessionConfig};
use scylla::transport::errors::{NewSessionError, QueryError};

use async_trait::async_trait;

use thiserror::Error; // TODO: Remove use for this

pub struct ScyllaManager {
    client_config: SessionConfig,
}

impl ScyllaManager {
    pub fn new (client_config: SessionConfig) -> ScyllaManager {
        ScyllaManager {
            client_config,
        }
    }
}

#[derive(Error, Debug)]
pub enum ScyllaR2D2Error{
    #[error("An error occurred while trying to create a new session")]
    NewSessionError(NewSessionError),
    #[error("An error occurred while trying to send a query")]
    QueryError(QueryError)
}

impl From<NewSessionError> for ScyllaR2D2Error {
    fn from(error: NewSessionError) -> Self {
        ScyllaR2D2Error::NewSessionError(error)
    }
}

impl From<QueryError> for ScyllaR2D2Error {
    fn from(error: QueryError) -> Self {
        ScyllaR2D2Error::QueryError(error)
    }
}

#[async_trait]
impl ManageConnection for ScyllaManager {
    type Connection = Session;
    type Error = ScyllaR2D2Error;

    async fn connect(&self) -> Result<Self::Connection, Self::Error> {
        let mut bld = SessionBuilder::new();
        bld.config = self.client_config.clone();

        Ok(futures::executor::block_on(bld.build())?)
    }

    async fn is_valid(&self, db: &mut bb8::PooledConnection<'_, Self>) -> Result<(), Self::Error> {
        match db.query("SELECT cql_version FROM system.local;", &[]).await {
            Ok(_) => {
                println!("Queried fine");
            },
            Err(e) => {
                println!("Query error: {}", e);

                return Err(ScyllaR2D2Error::QueryError(e));
            }
        }
        Ok(())
    }

    fn has_broken(&self, db: &mut Self::Connection) -> bool {
        // TODO
        false
    }
}

This is the pool configuration that causes issues (sorry for bad formatting)

    let cassandra_manager = bb8_scylla::ScyllaManager::new(cassandra_config);
    let db_pool = bb8::Pool::builder()
        .max_size(100)
        .min_idle(Some(15))
        .idle_timeout(Some(Duration::from_secs(900))) // 900 seconds = 15 minutes
        .connection_timeout(Duration::from_secs(10)) 
        .build(cassandra_manager)
        .await
        .unwrap();

The max_size and min_idle don't need to be that high, it can be set to something as low as 3 and 2 respectively and the issue still happens.

If you need any more information I'll be happy to provide. Thanks in advance!

@djc
Copy link
Owner

djc commented Nov 10, 2021

First, I'm a little confused by your issue. The title mentions when max_size is greater than the current amount of idle connections but your description refers to min_idle. Second, I guess I'm a little skeptical that you're problem is in bb8 itself. The library panics if max_size >= min_idle, so if you're the first person to run into this that would mean that no one else is using min_idle, right? Have you tried this with the default pool configuration?

The only code inside the pool implementation which refers to max_size is PoolInternals::approvals(), which decides how many additional connections should be allowed.

Given that I don't have a ScyllaDB, it would be great if you could try to reproduce this as a test case that doesn't depend on any actual database (you could have a look at how the tests to this). BTW, blocking in your ManageConnection::connect() method seems like it has the potential for causing issues.

Sorry, I'm not doing very active maintenance on bb8 right now, so I can't offer you much more support, though I'd be happy to review your reproducer and fix it if we do find an issue with the current pool implementation.

@SeanOMik
Copy link
Author

SeanOMik commented Nov 10, 2021

First, I'm a little confused by your issue. The title mentions when max_size is greater than the current amount of idle connections but your description refers to min_idle.

When I said current idle connections I meant min_idle.

Second, I guess I'm a little skeptical that you're problem is in bb8 itself. The library panics if max_size >= min_idle, so if you're the first person to run into this that would mean that no one else is using min_idle, right?

From what I saw, the library asserts that max_size >= min_idle, so it will only panic if its min_idle is larger than max_size.

Have you tried this with the default pool configuration?

I think I have, but I'll have to try it again to make sure, its been a few days.

Given that I don't have a ScyllaDB, it would be great if you could try to reproduce this as a test case that doesn't depend on any actual database (you could have a look at how the tests to this).

I'll work on making a minimal example that reproduces this error.

BTW, blocking in your ManageConnection::connect() method seems like it has the potential for causing issues.

Oops, thats left over from when I used r2d2 but then found out that bb8 exists.

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

2 participants