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

CustomizeConnection::on_release() #94

Merged
merged 2 commits into from
Dec 26, 2020
Merged

CustomizeConnection::on_release() #94

merged 2 commits into from
Dec 26, 2020

Conversation

djc
Copy link
Owner

@djc djc commented Dec 25, 2020

@agersant how do you feel about this?

@codecov
Copy link

codecov bot commented Dec 25, 2020

Codecov Report

Merging #94 (56ef326) into main (50e7e4f) will increase coverage by 1.41%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
+ Coverage   80.38%   81.80%   +1.41%     
==========================================
  Files           6        6              
  Lines         668      720      +52     
==========================================
+ Hits          537      589      +52     
  Misses        131      131              
Impacted Files Coverage Δ
bb8/src/inner.rs 83.84% <81.25%> (+2.17%) ⬆️
bb8/src/api.rs 66.31% <83.33%> (-0.71%) ⬇️
bb8/src/internals.rs 92.40% <100.00%> (ø)
bb8/tests/test.rs 87.75% <100.00%> (+1.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50e7e4f...56ef326. Read the comment docs.

@agersant
Copy link
Contributor

Looks good to me, thanks for following up on this!

Co-authored-by: Antoine Gersant <antoine.gersant@lesforges.org>
@djc djc merged commit 567f6f2 into main Dec 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the on-release branch December 26, 2020 20:39
@djc
Copy link
Owner Author

djc commented Dec 27, 2020

I ended up removing it before release, since I'm not confident in the mechanics (I found at least one gap which wasn't that easy/clean to solve), and I can't really think of a use case. Will leave this out for now and add it back if someone comes up with a use case.

@paolobarbolini
Copy link

paolobarbolini commented Mar 5, 2021

This is exactly what we need for lettre. We have a PR lettre/lettre#550 for adding a connection pool to our async version of the API, but we need a way for quit to be called before dropping a connection. on_release would be perfect for this. It's also what we use in the sync version of our API, based on r2d2

@djc
Copy link
Owner Author

djc commented Mar 5, 2021

Can you open a new issue and preferably a PR?

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.

3 participants