Adding an optional healthcheck feature

[jgnagy]

Adds an optional :healthcheck option to ConnectionPool that accepts a Lambda. Defaults to always returning true (thus basically is disabled by default) but allows executing arbitrary code (within the safety of the mutex) to determine if the object is healthy on demand just before returning the pool member.

I'll submit another commit tomorrow to update the documentation if this is acceptable.

This might address #125 depending on the use case.

[mperham]

I've stated from day one that the connection pool does not do health checks. I would rather have the feature explicitly not be a health check but rather "is the connection still needed?", i.e. run it when checking the connection back into the pool. Slightly different semantics, different expectations and should still be able to fulfill your need. wdyt?

[jgnagy]

I can see the utility in renaming this feature/option to :before_checkout rather than :healthcheck, then I can expand it with :before_checkin as well, offering the ability to run a distinct lambda as a sort of callback on each of those activities.

I realized that you called out the lack of health checking, but I didn't realize that was a design decision; I assumed it just wasn't something you needed to solve for yet.

How do you feel about this callback approach instead?

[mperham]

Callbacks are generally a bad idea IMO. What do you do if they raise an exception? What args and return value should they have?

If we want to reap connections, implement that internally so we don't expand the user-visible API footprint aside from a few config options.

[jgnagy]

I would rather have the feature explicitly not be a health check but rather "is the connection still needed?", i.e. run it when checking the connection back into the pool

Clearly I misinterpreted this statement since I thought a callback was the "it" we'd run when checking the connection back into the pool. Can you elaborate on what you meant by this so I can help implement it?

What do you do if they raise an exception? What args and return value should they have?

The approach I proposed operates on the connection object itself, takes no other arguments, and needs only to return something "truthy" to retain the connection. As for exceptions, I imagine you'd probably use the same approach you do now when you yield the connection to the user in #with:

        Thread.handle_interrupt(Exception => :immediate) do
          yield conn
        end

It is, after all, running user-provided code, so it should probably bubble up to the user to recover. Maybe even a friendly reminder to the user in the README to catch exceptions and return false in their lambda to abandon misbehaving connections.

This seems to be the cleanest approach to allowing arbitrary clients to implement a connection pool while only needing to describe how to tell if that connection is still usable. Without some means for the user instantiating the ConnectionPool to provide this description, how will the ConnectionPool know which connections to reap?

All that said, if you're not interested in this feature, by all means let me know. I don't want to pollute your project with code/features you're not interested in maintaining. I need this capability for clients that I'm not writing and I find replacing misbehaving clients this way easier than wrapping those clients and writing my own repair/reconnect logic, but I can work around it. I just wanted to add a small feature I was surprised didn't exist to a library I enjoy using.

[mperham]

I was thinking reap_idle_after: 300. The stack of connections might be ordered based on last usage, meaning you could check the last element in the stack and remove it if it has not be used within 300 seconds. We'd need to start tracking connection last_used_at.

Alternatively we could reap connections after they age beyond N seconds reap_older_than: 3600, irrespective of their use. In that sense the pool would refresh connections to keep them young. We would just need to track created_at.

Either of these checks could be implemented internally during the checkin process. I think reaping idle has a valid use case per #125, I can't see why a connection needs to be "young" so I'd favor that approach less. Just writing down thoughts...

wdyt?

[jgnagy]

A generic, time-based reaping of connections (whether it be by "max age" or "max idle") does serve as a decent catch-all for bad connections, but it is less than ideal since "idle" might not be representative of what the connection is actually doing in the background. Consider the use-case of connections that leverage keep-alives (like SSH connections). I do see how it might be helpful to save some memory if pool usage dies down.

That said, I'll take it if it is something you'd be willing to accept. I'll close this PR and open a fresh one based on this approach.

[mperham]

I think trimming idle connections has some value so I’d be happy to see a PR around that. Thanks for your patience in working through this with me.