Sniffing connection pool is now disposable

[garymcleanhall]

The SniffingConnectionPool contains a ReaderWriterLockSlim, which implements IDisposable. Because the connection pool wholly owns the lock, their lifetimes are linked. The SniffingConnectionPool should therefore allow deterministic disposal of the ReaderWriterLockSlim by implementing IDisposable.

The tests ensure that the interface is implemented, and that the pool is unusable after disposal. Could see no way of asserting that the lock is definitely disposed.

[garymcleanhall]

Fyi: I signed the CLA. Not sure why it says that I didn't.

[russcam]

Hey @garymcleanhall, I'm tidying up the branches to rename (branch move and delete)

• 2.0 to master
• develop (current 1.x client) to 1.x

and as a consequence, closed this PR as a result of deleting the remote develop branch, sorry! I've reinstated the branch to keep track of the open PRs but feel free to close this PR and open a new one with the change against 1.x 😄

[russcam]

I'm not quite seeing why the ReaderWriterLockSlim needs to be disposed in how it is used here (other than it implements IDisposable 😄 ).

Its lifetime is tied to SniffingConnectionPool and ultimately to ConnectionConfiguration and IElasticsearchClient (and as a consequence, IElasticClient). One client can exist for the lifetime of the application.

[garymcleanhall]

If an object implements IDisposable then it should be explicitly disposed. Here, the finalizer is being relied on to dispose of the ReaderWriterLockSlim's unmanaged resources. This will happen at some point in the future. Instead, it should be deterministically disposed via a using bock, for example.

Having said this, my solution is not right either. Dispose needs to be called on the ReadWriterBlockSlim, but not by adding IDispose to I will correct and reissue

[garymcleanhall]

Ugh, never comment when using a phone! Hopefully that is still mildly intelligible.

[garymcleanhall]

Reopening this with a slight change. Run code analysis on NEST and you'll see numerous violations of CA1001: https://msdn.microsoft.com/en-us/library/ms182172.aspx
This only tackles one of them. Fixes are all breaking changes but will prevent leaking unmanaged resources.

[Mpdreamz]

Hi @garymcleanhall thanks for opening this PR :)

I've opened another PR for this here: #1720 which adds disposable where it makes sense. Mainly ConnectionSettings and the objects (IConnectionPool, IConnection) it owns.

ConnectionSettings is expected to live as long as your application does but for scenarios where you might new one each time it does not hurt to be deterministic (although these cases should be rare).