Add connection pool capabilities

[mfenniak]

Not sure yet how this will operate, since connections can be used by multiple concurrent users without any issues. Seems like it will be necessary to first perform some testing to see why/when we should use multiple connections, and then find a way to translate those results into a connection pooling strategy.

[dragan]

I think this makes sense in situations where you have more than one node running. That way if one is down, we have another option to try to connect to. Basically it allow a user to specify all the nodes and using #67 allow them to also set a priority. But yeah, once we have a connection, we probably don't need another one if one can handle multiple concurrent users without any trouble.

[mfenniak]

My original thought was that the point of connection pooling was to increase performance (ie. more network connections) and to reduce contention on an individual connection. I think there's some value to both of these things, but as the issue describes, it would be necessary to do some testing to determine what value there is.

You're suggestion of implementing client-side high-availability in the connection pool is a good one. I like that approach.

I think I'd like to see "connection pooling" implemented as a one or more delegating implementations of IConnection & IAsyncConnection. For example:

• ReliableConnection: wraps every Run method with exception handling. On network errors, attempts to reconnect to the server, or connect to another server, and re-run the query. After a small configurable amount of failures, stops retrying.
• PooledConnection: internally uses more than one connection to reduce the single connection contention.

[dragan]

Yeah, this should give a very good fault tolerant client.

[mfenniak]

I've just pushed up an initial implementation of connection pooling. Here are some notable API changes:

• Changed connection factories to return open, ready-to-use connections.
• Moved methods related to establishing a connection off IConnection; this makes it logical that a connection factory returns an opened, ready-to-use connection.
• Added IDisposable to IConnection interface.
• Changed IConnectionFactory to no longer accept a cluster name; added an async interface.
• Changed RethinkDb.Configuration so that it will assembly a connection factory for a specific cluster name, using the public static entry-point ConfigurationAssembler.

Still to-do in this PR before merging:

• Unit tests for ConnectionPoolingConnectionFactory
• Add configuration elements to allow the assembled connection factory to be a connection pool factory.
• Create the reliable connection factory, as per previous comments in this issue
  • implementation
  • unit tests
  • configuration-based creation

[mfenniak]

Woah, this grew a bit larger than I anticipated, but I believe it is now done. At the end of it all, this provides a very simple persistent connection pool, and a very simple connection wrapper that detects network errors and retries connections. Configuration and APIs have changed to allow this to happen, and I think both of these components could become as sophisticated as required in the future.

There's a lot of code in this PR; it'd be nice if someone could do a code review if they have the time. At a minimum, maybe a release note review to see if there are any concerns about the direction I took this? @karlgrz, @dragan, @tetious, if someone has some time, I'd appreciate any comments you'd have. :-)

[karlgrz]

Taking a look now...

[karlgrz]

The only issue I see (after an admittedly cursory look) is here:

d6eba54#diff-1bdfd502a6060120e1f62968f20f9097R23

I would strongly suggest against locking directly on the connection pool, and instead use a separate dummy object to ensure that deadlocks are prevented when trying to update that pool.

Other than that I think it looks pretty good to me. It is a lot of code :-)

[mfenniak]

Hm... I'm not too concerned about that lock as it is on a private member variable. I don't see how it could possibly have the problem you're suggesting. Am I mistaken?

[karlgrz]

Yes.

Just because it's private doesn't guarantee that another thread doesn't try to grab a connection from the pool while another is writing to it. It's very unlikely to happen, but it will. And it's annoying to track down.

I've seen this before with Npgsql drivers in a multi-tenant web app. Locking on the connection pool collection directly caused some obnoxious debugging for us.

Just my observation, I'd be most comfortable with a completely separate lock object, but if you're comfortable omitting it until it becomes a problem that's ok too.

[mfenniak]

Just because it's private doesn't guarantee that another thread doesn't try to grab a connection from the pool while another is writing to it. It's very unlikely to happen, but it will. And it's annoying to track down.

Being private doesn't prevent that. But the lock on the pool object does prevent exactly that, and the fact that the object is private means that only the code in this class can access it, and I can easily verify that all the accessors to the object inside this class are protected by the same locking mechanism. Two threads, one attempting to read and one attempting to write, the first to acquire the lock will proceed and the second will wait.

The only difference that I'm aware of between this approach and using a sentinel object is if the LinkedList object itself ever performs a "lock (this)". It's the only other piece of code that could lock on the collection. But, "lock (this)" is customarily considered unsafe because of the potential of another thread to lock the object in an unexpected way; I'm making the assumption that LinkedList, a framework class, wouldn't be doing that.

I'm pretty confident that this code is fine, but I'd love a more detailed explanation of why I'm wrong, as it would suggest an important mistake in my understanding. :-) What am I missing? Can you cite the specific Npgsql bug that you've run into before?; I'm pretty familiar with that code base too.

[karlgrz]

I have a feeling, after reading your much more thorough explanation than mine, that somewhere in our code we were doing "lock (this)" instead of "lock (someObj)" which was causing the problems.

I think you're right about it being safe :-)

Looks good to me, I should have a chance to play around with it on mono later on the train home.

[mfenniak]

Thanks for taking the time to take a look, Karl. I appreciate your feedback. :-) I'm going to merge this ('cause why not), and if you have any additional comments if you have time to take a deeper look, we can address them post-merge.