Remove idle connections from pool #309
Conversation
There was a problem hiding this comment.
I'm fine with the added functionality, but I want to note the following caveats:
- Users can still get
MemcachedUnexpectedCloseError, even with this change. This is a distributed system, so it will be very easy for the timing to work out such that the connection looks "live" on the client side, but has become "idle" on the server side. - Users still need to expect and handle
MemcacheUnexpectedCloseError. I'm starting to think, after recent discussions, that we need to provide more guidance for how to do this. There is no way to guarantee that you will never get this exception.
With those caveats, I think this can eliminate some spurious cases of MemcacheUnexpectedCloseError, which should make normal error handling a little more stream-lined.
|
Thank you for your prompt review, and I apologize for my own delayed response. I agree with all your comments, and I'll work on updating the PR as soon as I find the time. |
Just wanted to check in and see if you were able to find the time to make these revisions. |
The Memcached server can be configured to drop idle connections (via the `idle_timeout` option). When this occurs, pymemcache may still try to use the connection, resulting in `MemcacheUnexpectedCloseError`. Hence the need for client-side idle timeout logic. This change was proposed in github.com/pinterest#114, but ultimately dropped. The current PR uses the same approach but differs slightly in the details.
jwgcarlson
force-pushed
the
pool-idle-timeout
branch
from
12119f3
to
e02840b
Compare
Oops, I definitely forgot about it, but made the requested changes now. Thanks for your patience. |
|
Thanks @jwgcarlson! |
The Memcached server can be configured to drop idle connections (via the
idle_timeoutoption). When this occurs, pymemcache may still try touse the connection, resulting in
MemcacheUnexpectedCloseError. Hencethe need for client-side idle timeout logic.
This change was proposed in #114, but ultimately dropped.
The current PR uses the same approach but differs slightly in the details.