Ensure inputs are closed on error. Add runtime GC finalizer as additional guard to close iterators

[stuartcarnie]

newMergeGuardIterator ensures inputs are closed if Merge returns error and wraps the returned merge iterator in <type>FinalizerIterator

• FinalizerIterator sets a runtime finalizer and calls Close
  when garbage collected. This will ensure any associated cursors
  are closed and ref counts on related TSM files released
• query.Iterators#Merge call could return an error and the inputs
  would not be closed, causing a cursor leak, hence the newMergeGuardIterator

Required for all non-trivial PRs

• Rebased/mergable
• Tests pass
• CHANGELOG.md updated

[stuartcarnie]

updated commits with debug code removed

[benbjohnson]

@stuartcarnie I haven't used SetFinalizer() before but there are quite a few concerns here: https://groups.google.com/forum/#!topic/golang-dev/DMiUkpS1uyQ

What's the goal?

[stuartcarnie]

@benbjohnson the primary goal is to address any consumers of CreateIterator failing to call Close and consequently leaking cursors. The ultimate issue is the ref count of the associated TSMFile objects never returns to 0 and subsequent compactions generate .tsm.tmp files until after a restart. The issue is we use a manual, reference counted mechanism to detemine usage of TSMFiles associated with seeks in the cursors. That means we rely on consumers to call Close to ensure the ref count is decremented. This is a guard until we can spend more time throughly reviewing the query iterators or refactoring the code. Naturally, this is based on the assumption that the iterator returned from Engine#CreateIterator is garbage collected and not referenced indefinitely due to a hung goroutine, etc.

---

I have read that post before. It was started by Dmitry because the Go GC was not a precise garbage collector, thus you could not rely on GC finalizers being called. That has since been rectified with 1.3. Rob Pike responded that it wasn't as gloomy as Dmitry made it out to be and later Rob clarified they were not going to be removed. They are used for a similar purpose in the net package to ensure the Close method of garbage collected file descriptors is called in order to free resources.

[e-dard]

I think we should make a concerted effort in the future to put the work in to weed out where calls to Close are getting missed, rather than relying on this approach.

My only nit with this PR is the naming for the iterators. They're still merge iterators, so I think they should be called XMergeIterator, whether X is Guard or something else.

Same thing for, for example, FloatFinalizerIterator. It's a type of FloatIterator, so it seems to me that it should be called, for example, FinalizedFloatIterator or something.

[stuartcarnie]

Agreed @e-dard, the long term goal would be to make sure Close is called.

The other struct names for the iterators in the iterator.gen.go file start with the data type (float, integer, etc), so I followed that convention

Q: Do you think we should change that?

query.Iterators(input).Merge(opt) returns query.Iterator and the <type>FinalizerIterator wraps any query.Iterator, including merge iterators or the various call iterators returned by the Merge function.

I'll rename the newMergeGuardIterator to newMergeFinalizerIterator. It is intended as a guard to make sure the inputs are closed if the call to query.Iterators(inputs).Merge(opt) returned an error. If it succeeds, it wraps the resulting query.Iterator in the Finalizer iterator.

[e-dard]

@stuartcarnie I think the changes are good. 🍏