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

Pool connections using bb8 #115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SafariMonkey
Copy link

@SafariMonkey SafariMonkey commented Jun 21, 2024

Moves from an async worker (bad abstraction) to a connection pool.

bb8 does not have an obvious way to kill connections involved in a panic, so as a (hopefully temporary) workaround, a wrapper is used that checks for panics on drop and, if so, replaces the connection with a None before it is returned to the pool, at which point it is identified as dead via has_broken.

Draft as it compiles but is not yet tested.

@SafariMonkey SafariMonkey marked this pull request as ready for review June 22, 2024 15:14
Copy link
Contributor

@TheButlah TheButlah left a comment

Choose a reason for hiding this comment

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

looks good overall. Left a few minor nits. Then afterwords I'll test. Don't forget to rebase on main.

use eyre::{bail, ensure, Context, OptionExt};
use async_trait::async_trait;
use eyre::{bail, ensure, eyre, Context, OptionExt};
use eyre::{ContextCompat, Result};
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer eyre::WrapErr instead of Context and eyre::OptionExt instead of ContextCompat

async fn is_valid(&self, framed: &mut Self::Connection) -> Result<(), Self::Error> {
let framed = framed
.as_mut()
.wrap_err("connection was dropped due to panic")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

use eyre::OptionExt

Comment on lines +75 to +76
let framed = Framed::new(bi);
Ok(Some(framed))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let framed = Framed::new(bi);
Ok(Some(framed))
Ok(Some(Framed::new(bi)))

async fn get_framed(&self) -> Result<DropConnectionOnPanic<'_>> {
let pooled_connection = self.pool.get().await.map_err(|e| match e {
bb8::RunError::User(eyre) => {
eyre.wrap_err("get from connection pool failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
eyre.wrap_err("get from connection pool failed")
eyre.wrap_err("could not get framed stream from connection pool")

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

Successfully merging this pull request may close these issues.

None yet

2 participants