Feature request: Allow connection pool to immediately disconnect allocated connections #442

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

2 participants

@dlee

There should be a way to disconnect currently running connections.

It could be a separate method disconnect_allocated, or an option into disconnect, ie. disconnect(:allocated => true).

@jeremyevans
Owner

The ShardedThreadedConnectionPool already allows handles Database#disconnect when connections are allocated. The default ThreadedConnectionPool does not do this by design, for performance reasons. You can use a :servers=>{} option when connecting to the database to use the sharded pool.

@dlee

I should have been more clear. This is a feature request to immediately disconnect a live allocated connection, not to remove an allocated connection upon completion.

@jeremyevans
Owner

I reject this feature request, as it leads to undefined behavior. You can't safely disconnect connections that another thread may be using. Depending on the underlying driver, attempting to disconnect a connection in use by another thread can lead to a segfault.

If you really want to disconnect currently allocated connections, you can grab the connection pool mutex, iterate over the allocated connections and disconnection each one manually. However, doing so is a terrible idea and should not be part of Sequel.

@dlee
@jeremyevans
Owner

It may work OK for your current driver (maybe because you are using JDBC, or maybe because you have just been getting lucky), but I can assure you there are many drivers that would not handle the situation gracefully. And I wouldn't consider that a bug in the driver. In many cases with the API you are proposing, disconnects are going to free resources that are still in use by another thread. IMO, what you are proposing is even more dangerous than Thread#raise, which will probably be removed in a future version of ruby because it is so dangerous.

@dlee

Actually, DB connection cancel is approach we are taking precisely because we thought it was safer than Thread#kill. Killing a DB connection would at least allow the Ruby state to be consistent.

I'm assuming that disconnecting a connection would only cut the connection, leaving the responsibility of resource cleanup to the owner thread. This is how it must be done for lower level disconnects (socket disconnects, for example), and I wouldn't think such disconnections would leave the SQL connection thread inconsistent.

Do you know of any documentation or blog that surfaces this issue? It seems like you've seen non-threadsafe disconnects in the works, and I'd rather learn from others' mistakes.

@jeremyevans
Owner

What Database#disconnect does depends on the adapter and driver. If the Sequel adapter calls a method on the driver that frees a related resource, then disconnecting connections in use by another thread will always be dangerous. If the method just marks the connection as disconnected and closes the socket, without freeing resources (leaving GC to free them), it's possible that disconnecting allocated connections could work. Many drivers written in C are specifically thread-unsafe in regards to connections, in that operations on connections by multiple threads simultaneously is undefined behavior.

Unless you've communicated directly with the JDBC driver authors and they believe it is safe to do what you are doing, I would recommend against your approach. Now, Thread#kill is bad too, so I'm not sure which is worse. I've honestly never had an app where disconnecting currently allocated connections was a requirement, so I can't say what I'd do in your place.

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