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

Returning connection from a pool without pool explicitly in scope #11

Closed
viraptor opened this Issue May 19, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@viraptor

viraptor commented May 19, 2015

I'm not very experienced in rust yet, so I'm sorry for potential misunderstanding ;)

I ran into an issue where I'd like to abstract returning a connection from a pool into a function. Basically the original code in Iron request handler is:

fn front_page_handler(req: &mut Request) -> IronResult<Response> {
    let pool = req.get::<Read<Database>>().ok().expect("database component not initialised");
    let connection = pool.get().unwrap();
    ...

and I wanted to make the first two lines just let connection = get_pool_connection(req);. This however is not possible unless pool is returned along with the connection so it can live for the same time.

I was hoping this could be improved by storing an Rc<Pool> somewhere in the connection itself. Or maybe there are other ways to allow a function like this to exist:

fn get_pool_connection(req: &mut Request) -> PooledConnection<T> {
    let pool = req.get::<Read<Database>>().ok().expect("database component not initialised");
    pool.get().unwrap()
}

Related SO with the question and answer here: https://stackoverflow.com/questions/30252054/refactoring-messes-up-mutable-borrow-why

@viraptor viraptor changed the title from Returning one connection from the pool to Returning connection from a pool without pool explicitly in scope May 19, 2015

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler May 20, 2015

Owner

You can do this with a small amount of trickiness:

struct SelfContainedConnection<M, T> {
    // must be in this order to ensure the connection drops before the pool
    conn: PooledConnection<'static, T>,
    pool: Arc<Pool<M>>,
}

impl<M, T> SelfContainedConnection<M, T> where M: ConnectionManager, T: ConnectionManager::Connection {
    pub fn new(pool: &Arc<Pool>) -> Result<SelfContainedConnection, GetTimeout> {
        let conn = try!(pool.get());
        let pool = pool.clone();
        Ok(SelfContainedConnection {
            conn: unsafe { mem::transmute(conn) },
            pool: pool
        })
    }
}

impl<M, T> Deref for SelfContainedConnection<M, T> {
    type Target = T;

    fn deref(&self) -> &T {
        &*self.conn
    }
}

We define a new smart pointer that holds both the original PooledConnection pointer as well as a reference to the pool to make sure it stays alive.

Owner

sfackler commented May 20, 2015

You can do this with a small amount of trickiness:

struct SelfContainedConnection<M, T> {
    // must be in this order to ensure the connection drops before the pool
    conn: PooledConnection<'static, T>,
    pool: Arc<Pool<M>>,
}

impl<M, T> SelfContainedConnection<M, T> where M: ConnectionManager, T: ConnectionManager::Connection {
    pub fn new(pool: &Arc<Pool>) -> Result<SelfContainedConnection, GetTimeout> {
        let conn = try!(pool.get());
        let pool = pool.clone();
        Ok(SelfContainedConnection {
            conn: unsafe { mem::transmute(conn) },
            pool: pool
        })
    }
}

impl<M, T> Deref for SelfContainedConnection<M, T> {
    type Target = T;

    fn deref(&self) -> &T {
        &*self.conn
    }
}

We define a new smart pointer that holds both the original PooledConnection pointer as well as a reference to the pool to make sure it stays alive.

@viraptor

This comment has been minimized.

Show comment
Hide comment
@viraptor

viraptor May 30, 2015

Which version of rust does that work on @sfackler? On nightly I get:

src/db_pool.rs:38:73: 38:102 error: `ConnectionManager::Connection` is not a trait
src/db_pool.rs:38 impl<M, T> SelfContainedConnection<M, T> where M: ConnectionManager, T: ConnectionManager::Connection {

I rewrote that with explicit Connection where needed and dropped T:

pub struct SelfContainedConnection<M> where M: ConnectionManager {
    // must be in this order to ensure the connection drops before the pool
    conn: PooledConnection<'static, M>,
    pool: Arc<Pool<M>>,
}

impl<M> SelfContainedConnection<M> where M: ConnectionManager {
    pub fn new(pool: &Arc<Pool<M>>) ⟶  Result<SelfContainedConnection<M>, GetTimeout> {
        let conn = try!(pool.get());
        let pool = pool.clone();
        Ok(SelfContainedConnection {
            conn: unsafe { mem::transmute(conn) },
            pool: pool
        })
    }
}

impl<M> Deref for SelfContainedConnection<M> where M: ConnectionManager {
    type Target = M::Connection;

    fn deref(&self) ⟶  &M::Connection {
        &*self.conn
    }
}

But that still fails with:

src/db_pool.rs:43:28: 43:42 error: cannot transmute to or from a type that contains type parameters in its interior [E0139]
src/db_pool.rs:43             conn: unsafe { mem::transmute(conn) },

viraptor commented May 30, 2015

Which version of rust does that work on @sfackler? On nightly I get:

src/db_pool.rs:38:73: 38:102 error: `ConnectionManager::Connection` is not a trait
src/db_pool.rs:38 impl<M, T> SelfContainedConnection<M, T> where M: ConnectionManager, T: ConnectionManager::Connection {

I rewrote that with explicit Connection where needed and dropped T:

pub struct SelfContainedConnection<M> where M: ConnectionManager {
    // must be in this order to ensure the connection drops before the pool
    conn: PooledConnection<'static, M>,
    pool: Arc<Pool<M>>,
}

impl<M> SelfContainedConnection<M> where M: ConnectionManager {
    pub fn new(pool: &Arc<Pool<M>>) ⟶  Result<SelfContainedConnection<M>, GetTimeout> {
        let conn = try!(pool.get());
        let pool = pool.clone();
        Ok(SelfContainedConnection {
            conn: unsafe { mem::transmute(conn) },
            pool: pool
        })
    }
}

impl<M> Deref for SelfContainedConnection<M> where M: ConnectionManager {
    type Target = M::Connection;

    fn deref(&self) ⟶  &M::Connection {
        &*self.conn
    }
}

But that still fails with:

src/db_pool.rs:43:28: 43:42 error: cannot transmute to or from a type that contains type parameters in its interior [E0139]
src/db_pool.rs:43             conn: unsafe { mem::transmute(conn) },
@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler May 30, 2015

Owner

I'm not sure it works on any version - I wrote it from memory. That transmute error can be worked around, but it requires more unsafe hackery than I'm really comfortable with.

The request for a lifetime-unrestricted PooledConnection has come up before (maybe from @alexcrichton?). It might make sense to completely drop the lifetime parameter from PooledConnection, but I'm not sure how I feel about that. In the meantime, I've added a Pool::get_arc function that acts like the one in the code snippet above but avoids unsafe code if you're storing the Pool in an Arc:

let pool: &Arc<Pool<M>> = my_pool;
let conn = Pool::get_arc(pool.clone()).unwrap(); // conn has a 'static lifetime

I can publish the update to crates.io if this seems reasonable to you.

9d786e3

Owner

sfackler commented May 30, 2015

I'm not sure it works on any version - I wrote it from memory. That transmute error can be worked around, but it requires more unsafe hackery than I'm really comfortable with.

The request for a lifetime-unrestricted PooledConnection has come up before (maybe from @alexcrichton?). It might make sense to completely drop the lifetime parameter from PooledConnection, but I'm not sure how I feel about that. In the meantime, I've added a Pool::get_arc function that acts like the one in the code snippet above but avoids unsafe code if you're storing the Pool in an Arc:

let pool: &Arc<Pool<M>> = my_pool;
let conn = Pool::get_arc(pool.clone()).unwrap(); // conn has a 'static lifetime

I can publish the update to crates.io if this seems reasonable to you.

9d786e3

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 31, 2015

Contributor

@sfackler yeah crates.io currently has similar code to get a PooledConnnection<'static>

Contributor

alexcrichton commented May 31, 2015

@sfackler yeah crates.io currently has similar code to get a PooledConnnection<'static>

@sfackler

This comment has been minimized.

Show comment
Hide comment
Owner

sfackler commented May 31, 2015

@reem

This comment has been minimized.

Show comment
Hide comment
@reem

reem Jun 3, 2015

cc me, this is an important API consideration for iron

reem commented Jun 3, 2015

cc me, this is an important API consideration for iron

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jun 3, 2015

Owner

PooledConnection is currently set up with a reference to the pool since that seems like the more "Rustic" thing to do, but it seems like it's more of a pain than it's worth. Pool already has an internal Arc that it shares with its worker threads, so it'd be easy to remove the lifetime constraint on PooledConnection. It does make it a bit more unclear of when the pool will actually clean itself up, but I'd imagine it lives indefinitely anyway in basically all use cases.

Thoughts?

Owner

sfackler commented Jun 3, 2015

PooledConnection is currently set up with a reference to the pool since that seems like the more "Rustic" thing to do, but it seems like it's more of a pain than it's worth. Pool already has an internal Arc that it shares with its worker threads, so it'd be easy to remove the lifetime constraint on PooledConnection. It does make it a bit more unclear of when the pool will actually clean itself up, but I'd imagine it lives indefinitely anyway in basically all use cases.

Thoughts?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 3, 2015

Contributor

I suspect that the "overhead" from using an Arc here is negligible due to a DB connection being such a heavyweight object, and I wouldn't personally be too surprised by the lifetime semantics here. I would never shut down a pool of connections until the entire app dies anyway, so it's already super long lasting and unlikely to hit weird scoping problems.

This pattern is in use elsewhere, though, for example in Servo's work stealing deque. The BufferPool is used to create Stealers and Workers, both of which have have a strong reference to the internal pool. Basically the original buffer pool can disappear but won't actually get deallocated until all stealers/workers are gone. (just a data point)

Contributor

alexcrichton commented Jun 3, 2015

I suspect that the "overhead" from using an Arc here is negligible due to a DB connection being such a heavyweight object, and I wouldn't personally be too surprised by the lifetime semantics here. I would never shut down a pool of connections until the entire app dies anyway, so it's already super long lasting and unlikely to hit weird scoping problems.

This pattern is in use elsewhere, though, for example in Servo's work stealing deque. The BufferPool is used to create Stealers and Workers, both of which have have a strong reference to the internal pool. Basically the original buffer pool can disappear but won't actually get deallocated until all stealers/workers are gone. (just a data point)

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jun 3, 2015

Owner

A somewhat related question is how sharing across threads should be handled. Right now, you put the Pool into an Arc, but Pool could impl Clone itself. This seems a bit weird to me, but it does appear to be what Servo's BufferedPool does.

Owner

sfackler commented Jun 3, 2015

A somewhat related question is how sharing across threads should be handled. Right now, you put the Pool into an Arc, but Pool could impl Clone itself. This seems a bit weird to me, but it does appear to be what Servo's BufferedPool does.

@viraptor

This comment has been minimized.

Show comment
Hide comment
@viraptor

viraptor Jun 12, 2015

Just tested get_arc() and it's exactly what I needed. Thanks!

I'm not sure what the last comment meant. Something like pool.get_arc() where it does Pool::get_arc(self.clone())? That would be nice too.

viraptor commented Jun 12, 2015

Just tested get_arc() and it's exactly what I needed. Thanks!

I'm not sure what the last comment meant. Something like pool.get_arc() where it does Pool::get_arc(self.clone())? That would be nice too.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jun 12, 2015

Owner

Right now, if you want to share a Pool between threads, you have to stick it into an Arc and then clone that:

let pool = Arc::new(Pool::new(...));
let pool2 = pool.clone();
thread::spawn(move || {
    let pool = pool2;
    // use the pool
});
// use the other pool

We could change Pool so that it acts like an Arc on its own:

let pool = Pool::new(...);
let pool2 = pool.clone();
thread::spawn(move || {
    let pool = pool2;
    // use the pool
});
// use the other pool

Pool itself is just a bare wrapper around an Arc internally, so there's something to be said for removing a level of indirection: https://github.com/sfackler/r2d2/blob/master/src/lib.rs#L114-L116. It's an unrelated idea to the PooledConnection lifetime issue.

Owner

sfackler commented Jun 12, 2015

Right now, if you want to share a Pool between threads, you have to stick it into an Arc and then clone that:

let pool = Arc::new(Pool::new(...));
let pool2 = pool.clone();
thread::spawn(move || {
    let pool = pool2;
    // use the pool
});
// use the other pool

We could change Pool so that it acts like an Arc on its own:

let pool = Pool::new(...);
let pool2 = pool.clone();
thread::spawn(move || {
    let pool = pool2;
    // use the pool
});
// use the other pool

Pool itself is just a bare wrapper around an Arc internally, so there's something to be said for removing a level of indirection: https://github.com/sfackler/r2d2/blob/master/src/lib.rs#L114-L116. It's an unrelated idea to the PooledConnection lifetime issue.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jun 23, 2015

Owner

I've made the changes discussed: PooledConnection no longer has a lifetime bound and Pool directly implements clone.

I'll be releasing those changes and some others as v0.6 in a couple days probably.

Owner

sfackler commented Jun 23, 2015

I've made the changes discussed: PooledConnection no longer has a lifetime bound and Pool directly implements clone.

I'll be releasing those changes and some others as v0.6 in a couple days probably.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jun 25, 2015

Owner

v0.6.0's been released.

Owner

sfackler commented Jun 25, 2015

v0.6.0's been released.

@sfackler sfackler closed this Jun 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment