ThreadedConnectionPool disconnect doesn't disconnect allocated connections #441

Closed
dlee opened this Issue Feb 14, 2012 · 6 comments

Comments

Projects
None yet
2 participants

dlee commented Feb 14, 2012

The comment for #release sounds like you intend for allocated connections to be disconnected upon release, but the code just returns the allocated connection back into the available connection pool.

Owner

jeremyevans commented Feb 15, 2012

See response for #442. And please don't create duplicate issues. :)

dlee commented Feb 15, 2012

This is not the same as #442. It's clearly a bug in the implementation or the comment since inside the code for ThreadedConnectionPool it says:

If the server or connection given is scheduled for disconnection, remove the connection instead of releasing it back to the pool.

(https://github.com/jeremyevans/sequel/blob/master/lib/sequel/connection_pool/threaded.rb#L139)

Owner

jeremyevans commented Feb 15, 2012

It's a bug in the documentation, I'll fix that. At one point, the threaded connection pool did handle disconnects, but for performance reasons, the functionality was moved to the sharded threaded pool when the pool was split.

dlee commented Feb 15, 2012

Do you remember how bad the performance impact was? As it stands, disconnect will disconnect available connections but retain connections that are currently running. It's an unintuitive behavior, to say the least. I think it's better to have disconnect properly disconnect if the performance impact isn't so bad.

That said, there is the option of using the ShardedThreadedConnectionPool. If you think the performance impact is not worth having true disconnect in ThreadedConnectionPool, then I would recommend getting rid of the disconnect method (or rename to disconnect_unused) in ThreadedConnectionPool.

Owner

jeremyevans commented Feb 15, 2012

The vast majority of Sequel users do not use disconnect when connections are currently in used. By far the most common case is disconnecting before fork in a multiprocess program (such as unicorn), when no connections are currently allocated.

IIRC, the performance impact wasn't major, but it was measurable. Considering 99% of Sequel users don't need to disconnect allocated connections, and for the 1% that do, using the sharded pool is simple, I think the current behavior makes sense.

If you still want the current behavior changed, you need to post on the sequel-talk Google Group with your rationale and get the community behind your proposal. The GitHub issue tracker should only be used for bug reports and pull requests. Feature requests and anything requiring discussion should go to the Google Group.

dlee commented Feb 15, 2012

Ah, thanks, I'll post to the Google Group.

danielb2 pushed a commit to danielb2/sequel that referenced this issue Mar 14, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment