Skip to content

Spike/dispose #1720

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 13, 2016
Merged

Spike/dispose #1720

merged 5 commits into from
Jan 13, 2016

Conversation

Mpdreamz
Copy link
Member

This PR add IDisposable to some of our moving parts that (may) need it.

Historically we did not do this because this parts are expected to live for the duration of your application but it does not hurt to be explicit.

SniffingConnectionPools holds a ref to ReaderWriterLock which needs to dispose some events.
Transport held a SemaphoreSlim which really holds a system handle.

The ownership of the semaphore (used for sniffing on startup) moves to ConnectionSettings with this PR because new ElasticClient() implicitly creates a new Transport() each time, eventhough the IConnectionPool interface is already resilient to this it means we create more Semaphores then strictly needed if folks are new'ing clients all the time.

Therefore this PR also introduces some documentation on the recommended life times of our objects.

See this test for more info

The interfaces implement IDisposable for it's Level1 disposables using the pattern as outlined by Stephen Cleary:

http://www.codeproject.com/Articles/29534/IDisposable-What-Your-Mother-Never-Told-You-About

No object disposed exceptions are thrown as the previous article explains:

Instead of checking for the disposed state and throwing an exception, I recommend supporting "undefined behaviour". Accessing a disposed object is the modern equivalent of accessing deallocated memory.

To help prevent getting into this state all the Dispose() methods are explicitly implemented.

More rants against the classical dispose pattern: http://blogs.msmvps.com/peterritchie/2013/01/20/the-dispose-pattern-as-an-anti-pattern/

@garymcleanhall
Copy link

Sweet, glad this is being sorted. Thanks!

@gmarz
Copy link
Contributor

gmarz commented Jan 13, 2016

Left one minor nitpick comment, but otherwise this looks good. Really like the documentation tests. 👍

@Mpdreamz
Copy link
Member Author

Good catch @gmarz updated 👍

@Mpdreamz Mpdreamz closed this Jan 13, 2016
@Mpdreamz Mpdreamz reopened this Jan 13, 2016
gmarz added a commit that referenced this pull request Jan 13, 2016
@gmarz gmarz merged commit 35b0bed into master Jan 13, 2016
@gmarz gmarz deleted the spike/dispose branch January 13, 2016 17:40
@gmarz gmarz added this to the 2.0.0-alpha2 milestone Jan 15, 2016
@gmarz gmarz added the Feature label Jan 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants