Fix close method on Connection to use this

[danielcompton]

This is a pretty subtle bug, you will only run into it when you try to close a connection which has queries outstanding (e.g. changefeeds) using the .close method on the Connection. Before this PR there were two subtly different code paths that a closing connection could take:

1. Calling the close function in rethinkdb.core will deref conn (which is a Connection that implements deref to deref it's internal conn atom) and destructure the keys of the internal conn atom.
   • When running send-stop-query on every outstanding query, we pass the Connection.
   • Deeper in the call stack at send-query, we access the connection atom (without derefing it) by calling (:conn conn) on the record.
   • We do our swap! to remove the :waiting token from the set and continue.
2. Calling the close method on the Connection directly will call the close function with the conn atom directly.
   • The conn atom will deref and destructure because it also implements deref.
   • However when we get down to send-query we are trying to call (:conn conn) on the atom which will return nil.
   • swap! will throw a NPE.

I'm not sure whether this is a breaking change or not, it fixes the behaviour of .close, so it's worth at least highlighting in the changelog and release notes.

There's two things we could do to make this kind of bug not happen in the future:

1. Move the close function inside the Connection. People should be able to call close and this will use the close method on the Connection. However in my limited testing this doesn't seem to be happening. I'm not quite sure why, I thought records defined their methods as functions in the ns they were defined in.
2. Give the connection many fields, rather than putting them all inside a single conn variable. The only things that should be mutable are the :waiting and :token, so they could go into atoms.

Thoughts?

[danielcompton]

Merging this change but keen to hear peoples thoughts on modifying the structure of the connection object on #51.

[danielytics]

My vote would go to: both.

If the connection is a record, I don't see any reason why not to directly give it the fields it needs (rather than assoc'ing them in during connection) and as you say, only waiting and token need to be mutable, so best to have only those mutable rather than everything.

Having close as part of this record also makes a lot of sense.

I thought records defined their methods as functions in the ns they were defined in

One thing I've seen a good bit is to define a protocol in a separate namespace and then when calling the function on the record, using fully qualified names for that namespace:

(require '[foo.bar.protocols :as p])
(p/close my-record)

Another common idiom I've seen is to prefix all protocol functions with - and then writing wrappers (that often perform additional checks):

(defprotocol Closable
  (-close [this]))

(defrecord Connection []
  Closable
  (-close [this] ...))

(defn close [conn]
  {:pre (satisfies? Closable conn)}
  (-close conn))

Not sure if either of them help this particular issue. I can take a look tomorrow if you like.

[danielcompton]

The issue I think is that the Closeable interface isn't defined in this file. Also possibly because it is a Java interface not a Clojure protocol there are some differences there.