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

Add hooks to CustomizeConnection to allow users to perform actions on connection checkout/in. #131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,37 @@ pub trait CustomizeConnection<C, E>: fmt::Debug + Send + Sync + 'static {
/// The default implementation does nothing.
#[allow(unused_variables)]
fn on_release(&self, conn: C) {}

/// Called with connections immediately after they are checked out from
/// the connection pool for later usage.
///
/// This method can be used to customize the behaviour of `ConnectionManager::is_valid`
/// for specific usages like testing or other manual preperations.
///
/// The default implementations simply returns the existing `is_valid_result`
///
/// # Errors
///
/// If this method returns an error, the connection will be discarded
#[allow(unused_variables)]
fn on_checkout(&self, conn: &mut C, is_valid_result: Result<(), E>) -> Result<(), E> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the use cases for wanting to interact with is_valid_result here and has_broken below?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot comment on general use cases here, as that is highly dependent on the ConnectionManager implementation and the underlying connections. Therefore I will only comment on diesel related use cases here.

For on_checkin I see two use cases:

  • Allow to checkin transactions with open transactions into the pool, which is sometimes useful for tests. (Diesel marks connections with open transactions as broken)
  • Trying to close open transactions and "unbreak" connections here

For on_checkout the main use case would be setting up things before checking out the connection, for example opening a dangling transaction. I've included the result of is_valid here for consistency with on_checkin, but I cannot present a diesel related use case to actually use this result. I would guess that there could be a theoretical connection implementation that allows to reconnect the internal connection if some check has failed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfackler Anything that's missing here?

is_valid_result
}

/// Called with connections immediately before they are released to the connection
/// pool
///
/// This method can be used to customize the behaviour of `ConnectionManager::has_broken`
/// for specific usages like testing or manual cleanup.
///
/// The default implementation simply returns the existing `has_broken` values
///
/// If this methods returns true, the connection will be discarded instead of returned
/// to the connection pool.
#[allow(unused_variables)]
fn on_checkin(&self, conn: &mut C, has_broken: bool) -> bool {
has_broken
}
}

/// A `CustomizeConnection` which does nothing.
Expand Down Expand Up @@ -463,7 +494,14 @@ where
drop(internals);

if self.0.config.test_on_check_out {
if let Err(e) = self.0.manager.is_valid(&mut conn.conn.conn) {
let is_valid_result = self.0.manager.is_valid(&mut conn.conn.conn);

if let Err(e) = self
.0
.config
.connection_customizer
.on_checkout(&mut conn.conn.conn, is_valid_result)
{
let msg = e.to_string();
self.0.config.error_handler.handle_error(e);
// FIXME we shouldn't have to lock, unlock, and relock here
Expand Down Expand Up @@ -495,6 +533,11 @@ where

// This is specified to be fast, but call it before locking anyways
let broken = self.0.manager.has_broken(&mut conn.conn);
let broken = self
.0
.config
.connection_customizer
.on_checkin(&mut conn.conn, broken);

let mut internals = self.0.internals.lock();
if broken {
Expand Down