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

Support for discarding broken resources #3

Open
jebrosen opened this issue Nov 6, 2019 · 3 comments
Open

Support for discarding broken resources #3

jebrosen opened this issue Nov 6, 2019 · 3 comments

Comments

@jebrosen
Copy link

jebrosen commented Nov 6, 2019

Other pools have a way to signal that a resource is "broken" and a fresh connection is needed:

I believe this functionality can not be implemented on top of this crate's current API because there's no way to remove a resource from the pool, e.g. with a remove() function or into_inner() which would prevent the resource from being returned to the pool.

@khionu
Copy link
Owner

khionu commented Nov 11, 2019

There's a number of ways to implement this concept, but I'm not sure which is most ideal. A few of my thoughts:

  • AsyncPoolGuard::mark_broken function that simply drops the resource and signals for the creation of a new one from a registered factory
  • AsyncPoolGuard::recycle function which drops the resource and immediately creates a new one (maybe with a recycle_with that overrides the registered factory)
  • The factory could also be responsible for health checks. This would add a trait like tokio_resource_pool's Manage

@jebrosen
Copy link
Author

A callable function on the guard makes it the responsibility of the user of the guard to check for or deal with brokenness, while putting it on the trait makes it the responsibility of the implementer of the trait. It's possible to implement an abstraction over the first that works like the second; I have not put much thought into the other way around. For one of my use cases (i.e. "could Rocket use async_pool"), the trait is more convenient but guard methods should be usable.

There is also the issue of when brokenness is checked: a connection that idles long enough could become broken due to that, at which point it needs to be recycled upon being retrieved. I have not looked at when which libraries do their respective checks, but several of them have both a "fast check" ("definitely broken" vs undetermined) and a "slow check" ("definitely usable" vs not).

Of course, "brokenness" is a property that applies to and is necessary to handle resources such as database connections or file handles but not might apply to other resources that are worth pooling. I opened this issue with databases in mind, but this library looks to be geared towards more general use than databases.

@khionu
Copy link
Owner

khionu commented Nov 19, 2019

The compiler is smart enough to optimize out calls to default method bodies on traits that are empty. So, despite this being general purpose, a trait could still make for no overhead for more simple resources.

I'm starting to lean towards using the trait, only. An is_sound function (slow check) on the Manager trait called on rsvp, and an is_broken function (fast check) on guard drop. Make it the pool's responsibility to ensure sound resources.

Question this brings up is if I should require the Manager trait to replace broken resources, or if I should return an error on rsvp when the count hits 0. I don't like the latter, but I'm not sure if the former is reasonable.

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