Fix code review issues #2

Author: Michael Penick

cpp-driver/src/connection.hpp

@@ -83,7 +83,7 @@ class ConnectionListener {
 };
 
 /**
- * A connection. It's a socket wrapper that handles Cassadndra/DSE specific
+ * A connection. It's a socket wrapper that handles Cassandra/DSE specific
  * functionality such as decoding responses and heartbeats. It can not be
  * connected directly instead use a Connector object.
  *

cpp-driver/src/connection_pool_manager.cpp

@@ -98,9 +98,11 @@ void ConnectionPoolManager::add(const Address& address) {
 }
 
 void ConnectionPoolManager::remove(const Address& address) {
-  ScopedWriteLock rl(&rwlock_);
+  ScopedReadLock rl(&rwlock_);
   ConnectionPool::Map::iterator it = pools_.find(address);
   if (it == pools_.end()) return;
+  // The connection pool will remove itself from the manager when all of its
+  // connections are closed.
   it->second->close();
 }
 
@@ -141,7 +143,8 @@ void ConnectionPoolManager::add_pool(const ConnectionPool::Ptr& pool, Protected)
 }
 
 void ConnectionPoolManager::remove_pool(ConnectionPool* pool, Protected) {
-  internal_remove_pool(pool->address());
+  ScopedWriteLock wl(&rwlock_);
+  internal_remove_pool(wl, pool->address());
 }
 
 void ConnectionPoolManager::notify_up(ConnectionPool* pool, Protected) {
@@ -163,20 +166,23 @@ void ConnectionPoolManager::notify_critical_error(ConnectionPool* pool,
   if (listener_ != NULL) {
     listener_->on_critical_error(pool->address(), code, message);
   }
-  internal_remove_pool(pool->address());
+  ScopedWriteLock wl(&rwlock_);
+  internal_remove_pool(wl, pool->address());
 }
 
 void ConnectionPoolManager::internal_add_pool(const ConnectionPool::Ptr& pool) {
   LOG_DEBUG("Adding pool for host %s", pool->address().to_string().c_str());
   pools_[pool->address()] = pool;
 }
 
-void ConnectionPoolManager::internal_remove_pool(const Address& address) {
-  ScopedWriteLock wl(&rwlock_);
+void ConnectionPoolManager::internal_remove_pool(ScopedWriteLock& wl,
+                                                 const Address& address) {
   pools_.erase(address);
   maybe_closed(wl);
 }
 
+// This must be the last call in a function because it can potentially
+// deallocate the manager.
 void ConnectionPoolManager::maybe_closed(ScopedWriteLock& wl) {
   if (is_closing_ && pools_.empty()) {
     wl.unlock(); // The manager is destroyed in this step it must be unlocked

cpp-driver/src/connection_pool_manager.hpp

@@ -221,7 +221,8 @@ class ConnectionPoolManager : public RefCounted<ConnectionPoolManager> {
 
 private:
   void internal_add_pool(const ConnectionPool::Ptr& pool);
-  void internal_remove_pool(const Address& address);
+  void internal_remove_pool(ScopedWriteLock& wl,
+                            const Address& address);
   void maybe_closed(ScopedWriteLock& wl);
 
 private:

cpp-driver/src/connector.cpp

@@ -193,6 +193,11 @@ Connector* Connector::with_metrics(Metrics* metrics) {
 
 Connector* Connector::with_settings(const ConnectionSettings& settings) {
   settings_ = settings;
+  // Only use hostname resolution if actually required for SSL or
+  // authentication.
+  settings_.socket_settings.hostname_resolution_enabled
+      = settings.socket_settings.hostname_resolution_enabled &&
+      (settings.auth_provider || settings.socket_settings.ssl_context);
   return this;
 }
 

cpp-driver/src/pooled_connection.cpp

@@ -41,7 +41,7 @@ class RunClose : public Task {
 
 /**
  * A request callback that sets the keyspace then runs the original request
- * callback. The happens when the current keyspace wasn't set or has been
+ * callback. This happens when the current keyspace wasn't set or has been
  * changed.
  */
 class ChainedSetKeyspaceCallback : public SimpleRequestCallback {

cpp-driver/src/request_callback.cpp

@@ -234,13 +234,13 @@ ChainedRequestCallback::ChainedRequestCallback(const String& key, const Request:
   , has_pending_(false)
   , key_(key) { }
 
-ChainedRequestCallback::Ptr ChainedRequestCallback::chain(const String&key, const String& query) {
+ChainedRequestCallback::Ptr ChainedRequestCallback::chain(const String& key, const String& query) {
   has_pending_ = true;
   return ChainedRequestCallback::Ptr(
                    Memory::allocate<ChainedRequestCallback>(key, query, Ptr(this)));
 }
 
-ChainedRequestCallback::Ptr ChainedRequestCallback::chain(const String&key, const Request::ConstPtr& request) {
+ChainedRequestCallback::Ptr ChainedRequestCallback::chain(const String& key, const Request::ConstPtr& request) {
   has_pending_ = true;
   return ChainedRequestCallback::Ptr(
         Memory::allocate<ChainedRequestCallback>(key, request, Ptr(this)));

cpp-driver/src/request_handler.hpp

@@ -170,7 +170,6 @@ class RequestHandler : public RefCounted<RequestHandler> {
 
   void start_request(uv_loop_t* loop, Protected);
 
-  void add_execution(RequestExecution* request_execution, Protected);
   void add_attempted_address(const Address& address, Protected);
 
   void notify_result_metadata_changed(const String& prepared_id,

cpp-driver/src/socket_connector.cpp

@@ -199,8 +199,8 @@ void SocketConnector::handle_connect(TcpConnector* tcp_connector) {
 #if defined(HAVE_NOSIGPIPE) && UV_VERSION_MAJOR >= 1
     uv_tcp_t* tcp = &socket_->tcp_;
 
-    // This must be done after socket for the socket file descriptor to be
-    // valid.
+    // This must be done after the socket is intialized, which is done in the
+    // connection process, for the socket file descriptor to be valid.
     uv_os_fd_t fd = 0;
     int enabled = 1;
     if (uv_fileno(reinterpret_cast<uv_handle_t*>(tcp), &fd) != 0 ||