RecycleError has no purpose — is this intentional?

[SabrinaJewson]

I was always mystified by the algorithm used by managed::Pool to determine what happens when recycling fails, since it’s not documented. If one checks the definition of the Manager trait they will see that the recycle method takes a unique reference to an object and gives a RecycleResult<Self::Error>, where RecycleError is:

pub enum RecycleError<E> {
    Message(String),
    StaticMessage(&'static str),
    Backend(E),
}

The docs contain no information that helps here, so I’m left to question: Why? And after lots of reasoning and reading the docs, there is only one conclusion I could come to: It doesn’t make any sense at all. Let’s look at the thought process.

The recycle method chooses to give a RecycleResult<Self::Error> instead of a Result<(), Self::Error>. If we check the docs of Manager::Error, we see:

Error that this Manager can return when creating and/or recycling Objects.

However, if we check the docs of RecycleError::Message, we see:

Recycling failed for some other reason.

So, we can logically conclude that we should return RecycleError::Message in cases where:

• recycling the object succeeded (because no Manager::Error was returned)
• but recycling the object failed for reason other than recycling failing, as RecycleError::Message was returned

This is not a situation that can ever happen, as it’s directly contradictory, so we can logically conclude that RecycleError::Message (and StaticMessage) should never be returned. In that case, why have it, why is this a feature of the library?

Okay, well under the assumption that the library’s design makes sense, maybe I misread “some other reason” and it in fact “other reason” did not refer to recycling failing, but rather referred to something else. The other enum variant available is called “backend”, and says it is for an “Error caused by the backend”. “Backend” isn’t a well-defined term in this context, so we have to guess what it means. Well, we know that:

• create can only fail because of the backend;
• recycle can fail because of the backend, or because something else happened.

What does recycle do more than create that means it can fail in ways not caused by the backend? Well, it depends on the implementation. But at its face value, the proposition makes no sense: given that the error type of create is Self::Error it is most logical to assume that a backend error represents the creation of the resource failing. But the implementation of recycle quite explicitly should not be creating resources, so for what reason would the backend fail in it anyway?

Maybe “backend” represents any interaction with whatever entity is providing the resources; but then why is it assumed that, in the general case, recycle does more than just that interaction than create does? Why wouldn’t we also have a CreateError if we wished to enforce this “backend” model upon implementations of Manager?

So clearly this route is not the correct one. Let’s take an alternate angle: maybe the library is (internally and opaquely, because this is definitely not documented) making use of the structural details provided by the RecycleError enum somehow, so it’s matching on the enum and deciding what control flow path to take. If we check the current source code however, we’ll see that it doesn’t do this in .get(), in fact it does this nowhere in the library, nor is that structural information ever bubbled up to the user — the entire RecycleError is fully discarded without logging. This means the only logical conclusion is that the library is planning on using this information in a future version, it’s just not implemented.

There don’t seem to be any comments or issues that hint to that though, so I’m guessing it’s some secret plan of the maintainer — the only problem is that I don’t know how I can best prepare for that change when it comes around, because as seen above the docs of RecycleError are too sparse to know which error I should be giving in what scenario.

---

At this point, I gave up and chose to file this issue. I really cannot conceptualize what this type is and what it’s used for, and so I don’t know how to correctly use the library.

If the intention of the library author is that it is an API stability guarantee that RecycleErrors are ignored, I would suggest we use an alternative design to make this more explicit:

/// An error in recycling.
///
/// These errors are ignored if returned from [`Manager::recycle`].
pub struct RecycleError;

// in `Manager`
/// Tries to recycle an instance of [`Manager::Type`].
///
/// # Errors
///
/// Returns [`RecycleError`] if the instance shouldn’t be recycled, and the pool should
/// instead move on and attempt to create a new instance.
async fn recycle(&self, obj: &mut Self::Type) -> RecycleResult;

The fact that RecycleError is a unit struct makes it very explicit to the user that the library does not care what information it contains, and will simply ignore all data inside it. The user is encouraged to log errors if they care about them, or drop them if they don’t.

If the intention of the library author is for RecycleError::Message to have legitimately different semantic meaning from RecycleError::Backend, it would be very helpful to document this, and I would be willing to submit a PR for that if explained to me.

If the intention of the library author is to eventually introduce logging or something similar so that recycling errors aren’t fully ignored, I would suggest the following design for Manager instead:

pub trait Manager: Sync + Send {
    type Type;
    type Error;
    async fn create(&self) -> Result<Self::Type, Self::Error>;

    type RecycleError: Into<Box<dyn Error>>;
    async fn recycle(&self, obj: &mut Self::Type) -> Result<(), Self::RecycleError>;
}

This gives more flexibility on the part of the implementor of Manager to use whichever error type is most conveniënt for them.

[bikeshedder]

the entire RecycleError is fully discarded without logging. This means the only logical conclusion is that the library is planning on using this information in a future version, it’s just not implemented.

You're 100% right. I was planning to add some more meaningful logs to the library but never came around to implement it. Discarding the RecycleError without doing anything useful with it always bothered me as well. I was hoping for somewone with better knowledge to the tracing library to jump in and tackle this task:

• Add tokio-tracing #174

Regarding the enum variants it boils down to this:

• Backend is used when the recycle operation uses the backend. A good example is deadpool-postgres with a validation method other than Fast which does issue commands using the database client.
• Message and StaticMessage are there to allow for some custom messages. Think of a backend that discards objects after N uses, or uses some other metric. Nowadays I would probably change that to a Message(Cow<'static, str>) instead.

I was never very happy about that enum either but after a bunch of bikeshedding I just left it as is.

Now that I think about this I guess a better approach would be to change this into a struct like that:

/// This error type is returned by the [`Manager::recycle()`] method. Recycle errors
/// are never returned to the user of the library but are only used for logging and
/// possibly some advanced pool metrics in the future.
///
/// [`Manager::recycle()`]: super::Manager::recycle
#[derive(Debug)]
pub struct RecycleError {
    /// An optional error message explaining the reason why the recycling failed.
    pub message: Option<Cow<'static, str>>,
    /// An optional cause for this error.
    pub cause: Option<Box<dyn Error>>,
}

The Backend variant is flawed to begin with. Depending on the backend the Self:::Error might not be the one returned by the recycle operation so it's pretty much useless anyways.

[SabrinaJewson]

Thanks for the fast response, and the explanations — it’s a lot clearer now!

The struct you posted would be good, I think, but it also is effectively the same as the Error trait, just as a struct (.cause() = .source() and message = Display). That’s where the last design in the issue above comes from, effectively: just replace the concrete type with an associated type that implements Into<Box<dyn Error>>. This is the same pattern hyper uses quite a lot as well.

The reason I think using an Error associated type is more helpful than a RecycleError struct is that if users want to place something like anyhow::Error there it becomes totally frictionless, where the message would be provided by .context("here is why recycling failed") calls.

[bikeshedder]

Using the Error trait directly would be just as fine. The only gripe I have with that is the extra allocation necessary when using a Box<dyn Error> as return type. With the struct it's possible to abort recycling without providing any details and without the need for an allocation.

It's a micro optimization and I might be overthinking this a lot. Recycle errors are supposed to happen rarely and if those are ever going to be a problem you got some other issues to deal with anyways.

[bikeshedder]

I moved this issue to the 0.10 Milestone as I want to tackle all API breaking changes in the next release.

I'm a bit undecided if I RecycleError should be a struct or just a Box<dyn Error> instead.

[SabrinaJewson]

One advantage of using an impl Into<Box<dyn Error>> (or just a Box<dyn Error>) is that it prevents users from giving a “non-specific” error with RecycleError { message: None, cause: None }. If you force an implementation of the Error trait, it’s always that case that at least some meaningful message is returned.

[bikeshedder]

I moved that to the 0.11 Milestone as I want to get 0.10 out of the door. Right now it's merely cosmetic and should not block the 0.10 release.