Merge topic 'libuv-raii'

1e9b7d3c server: Switched to a auto model for handles
f43b9219 tests: Added tests to verify UV RAII semantics/constructs
a3abb85c Add RAII handles for libuv handle types
90f8db26 tests: unconditionally enabled server tests
b56b51fc utility: Disabled copy ctors in thread classes

Acked-by: Kitware Robot <[email protected]>
Merge-request: !1453

Source/CMakeLists.txt

@@ -1029,6 +1029,7 @@ list(APPEND _tools cmake)
 target_link_libraries(cmake CMakeLib)
 
 add_library(CMakeServerLib
+  cmUVHandlePtr.h cmUVHandlePtr.cxx
   cmConnection.h cmConnection.cxx
   cmFileMonitor.cxx cmFileMonitor.h
   cmPipeConnection.cxx cmPipeConnection.h

Source/cmConnection.cxx

@@ -26,7 +26,7 @@ void cmEventBasedConnection::on_alloc_buffer(uv_handle_t* handle,
 void cmEventBasedConnection::on_read(uv_stream_t* stream, ssize_t nread,
                                      const uv_buf_t* buf)
 {
-  auto conn = reinterpret_cast<cmEventBasedConnection*>(stream->data);
+  auto conn = static_cast<cmEventBasedConnection*>(stream->data);
   if (conn) {
     if (nread >= 0) {
       conn->ReadData(std::string(buf->base, buf->base + nread));
@@ -55,7 +55,7 @@ void cmEventBasedConnection::on_write(uv_write_t* req, int status)
 void cmEventBasedConnection::on_new_connection(uv_stream_t* stream, int status)
 {
   (void)(status);
-  auto conn = reinterpret_cast<cmEventBasedConnection*>(stream->data);
+  auto conn = static_cast<cmEventBasedConnection*>(stream->data);
 
   if (conn) {
     conn->Connect(stream);
@@ -76,7 +76,7 @@ void cmEventBasedConnection::WriteData(const std::string& _data)
 #endif
 
   auto data = _data;
-  assert(this->WriteStream);
+  assert(this->WriteStream.get());
   if (BufferStrategy) {
     data = BufferStrategy->BufferOutMessage(data);
   }
@@ -87,8 +87,7 @@ void cmEventBasedConnection::WriteData(const std::string& _data)
   req->req.data = this;
   req->buf = uv_buf_init(new char[ds], static_cast<unsigned int>(ds));
   memcpy(req->buf.base, data.c_str(), ds);
-  uv_write(reinterpret_cast<uv_write_t*>(req),
-           static_cast<uv_stream_t*>(this->WriteStream), &req->buf, 1,
+  uv_write(reinterpret_cast<uv_write_t*>(req), this->WriteStream, &req->buf, 1,
            on_write);
 }
 
@@ -156,13 +155,11 @@ bool cmConnection::OnServeStart(std::string* errString)
 
 bool cmEventBasedConnection::OnConnectionShuttingDown()
 {
-  if (this->WriteStream) {
+  if (this->WriteStream.get()) {
     this->WriteStream->data = nullptr;
   }
-  if (this->ReadStream) {
-    this->ReadStream->data = nullptr;
-  }
-  this->ReadStream = nullptr;
-  this->WriteStream = nullptr;
+
+  WriteStream.reset();
+
   return true;
 }

Source/cmConnection.h

@@ -5,6 +5,7 @@
 
 #include "cmConfigure.h" // IWYU pragma: keep
 
+#include "cmUVHandlePtr.h"
 #include "cm_uv.h"
 
 #include <cstddef>
@@ -107,8 +108,6 @@ public:
   bool OnConnectionShuttingDown() override;
 
   virtual void OnDisconnect(int errorCode);
-  uv_stream_t* ReadStream = nullptr;
-  uv_stream_t* WriteStream = nullptr;
 
   static void on_close(uv_handle_t* handle);
 
@@ -119,6 +118,8 @@ public:
   }
 
 protected:
+  cm::uv_stream_ptr WriteStream;
+
   std::string RawReadBuffer;
 
   std::unique_ptr<cmConnectionBufferStrategy> BufferStrategy;

Source/cmPipeConnection.cxx

@@ -2,6 +2,8 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmPipeConnection.h"
 
+#include <algorithm>
+
 #include "cmServer.h"
 
 cmPipeConnection::cmPipeConnection(const std::string& name,
@@ -13,39 +15,33 @@ cmPipeConnection::cmPipeConnection(const std::string& name,
 
 void cmPipeConnection::Connect(uv_stream_t* server)
 {
-  if (this->ClientPipe) {
+  if (this->WriteStream.get()) {
     // Accept and close all pipes but the first:
-    uv_pipe_t* rejectPipe = new uv_pipe_t();
+    cm::uv_pipe_ptr rejectPipe;
+
+    rejectPipe.init(*this->Server->GetLoop(), 0);
+    uv_accept(server, rejectPipe);
 
-    uv_pipe_init(this->Server->GetLoop(), rejectPipe, 0);
-    uv_accept(server, reinterpret_cast<uv_stream_t*>(rejectPipe));
-    uv_close(reinterpret_cast<uv_handle_t*>(rejectPipe),
-             &on_close_delete<uv_pipe_t>);
     return;
   }
 
-  this->ClientPipe = new uv_pipe_t();
-  uv_pipe_init(this->Server->GetLoop(), this->ClientPipe, 0);
-  this->ClientPipe->data = static_cast<cmEventBasedConnection*>(this);
-  auto client = reinterpret_cast<uv_stream_t*>(this->ClientPipe);
-  if (uv_accept(server, client) != 0) {
-    uv_close(reinterpret_cast<uv_handle_t*>(client),
-             &on_close_delete<uv_pipe_t>);
-    this->ClientPipe = nullptr;
+  cm::uv_pipe_ptr ClientPipe;
+  ClientPipe.init(*this->Server->GetLoop(), 0,
+                  static_cast<cmEventBasedConnection*>(this));
+
+  if (uv_accept(server, ClientPipe) != 0) {
     return;
   }
-  this->ReadStream = client;
-  this->WriteStream = client;
 
-  uv_read_start(this->ReadStream, on_alloc_buffer, on_read);
+  uv_read_start(ClientPipe, on_alloc_buffer, on_read);
+  WriteStream = std::move(ClientPipe);
   Server->OnConnected(this);
 }
 
 bool cmPipeConnection::OnServeStart(std::string* errorMessage)
 {
-  this->ServerPipe = new uv_pipe_t();
-  uv_pipe_init(this->Server->GetLoop(), this->ServerPipe, 0);
-  this->ServerPipe->data = static_cast<cmEventBasedConnection*>(this);
+  this->ServerPipe.init(*this->Server->GetLoop(), 0,
+                        static_cast<cmEventBasedConnection*>(this));
 
   int r;
   if ((r = uv_pipe_bind(this->ServerPipe, this->PipeName.c_str())) != 0) {
@@ -53,8 +49,8 @@ bool cmPipeConnection::OnServeStart(std::string* errorMessage)
       ": " + uv_err_name(r);
     return false;
   }
-  auto serverStream = reinterpret_cast<uv_stream_t*>(this->ServerPipe);
-  if ((r = uv_listen(serverStream, 1, on_new_connection)) != 0) {
+
+  if ((r = uv_listen(this->ServerPipe, 1, on_new_connection)) != 0) {
     *errorMessage = std::string("Internal Error listening on ") +
       this->PipeName + ": " + uv_err_name(r);
     return false;
@@ -65,18 +61,11 @@ bool cmPipeConnection::OnServeStart(std::string* errorMessage)
 
 bool cmPipeConnection::OnConnectionShuttingDown()
 {
-  if (this->ClientPipe) {
-    uv_close(reinterpret_cast<uv_handle_t*>(this->ClientPipe),
-             &on_close_delete<uv_pipe_t>);
+  if (this->WriteStream.get()) {
     this->WriteStream->data = nullptr;
   }
-  uv_close(reinterpret_cast<uv_handle_t*>(this->ServerPipe),
-           &on_close_delete<uv_pipe_t>);
 
-  this->ClientPipe = nullptr;
-  this->ServerPipe = nullptr;
-  this->WriteStream = nullptr;
-  this->ReadStream = nullptr;
+  this->ServerPipe.reset();
 
   return cmEventBasedConnection::OnConnectionShuttingDown();
 }

Source/cmPipeConnection.h

@@ -4,6 +4,7 @@
 
 #include "cmConfigure.h" // IWYU pragma: keep
 
+#include "cmUVHandlePtr.h"
 #include <string>
 
 #include "cmConnection.h"
@@ -23,6 +24,5 @@ public:
 
 private:
   const std::string PipeName;
-  uv_pipe_t* ServerPipe = nullptr;
-  uv_pipe_t* ClientPipe = nullptr;
+  cm::uv_pipe_ptr ServerPipe;
 };

Source/cmServer.cxx

@@ -22,13 +22,14 @@
 
 void on_signal(uv_signal_t* signal, int signum)
 {
-  auto conn = reinterpret_cast<cmServerBase*>(signal->data);
+  auto conn = static_cast<cmServerBase*>(signal->data);
   conn->OnSignal(signum);
 }
 
 static void on_walk_to_shutdown(uv_handle_t* handle, void* arg)
 {
   (void)arg;
+  assert(uv_is_closing(handle));
   if (!uv_is_closing(handle)) {
     uv_close(handle, &cmEventBasedConnection::on_close);
   }
@@ -58,6 +59,8 @@ cmServer::cmServer(cmConnection* conn, bool supportExperimental)
 
 cmServer::~cmServer()
 {
+  Close();
+
   for (cmServerProtocol* p : this->SupportedProtocols) {
     delete p;
   }
@@ -409,7 +412,7 @@ void cmServer::StartShutDown()
 
 static void __start_thread(void* arg)
 {
-  auto server = reinterpret_cast<cmServerBase*>(arg);
+  auto server = static_cast<cmServerBase*>(arg);
   std::string error;
   bool success = server->Serve(&error);
   if (!success || error.empty() == false) {
@@ -417,22 +420,19 @@ static void __start_thread(void* arg)
   }
 }
 
-static void __shutdownThread(uv_async_t* arg)
-{
-  auto server = reinterpret_cast<cmServerBase*>(arg->data);
-  on_walk_to_shutdown(reinterpret_cast<uv_handle_t*>(arg), nullptr);
-  server->StartShutDown();
-}
-
 bool cmServerBase::StartServeThread()
 {
   ServeThreadRunning = true;
-  uv_async_init(&Loop, &this->ShutdownSignal, __shutdownThread);
-  this->ShutdownSignal.data = this;
   uv_thread_create(&ServeThread, __start_thread, this);
   return true;
 }
 
+static void __shutdownThread(uv_async_t* arg)
+{
+  auto server = static_cast<cmServerBase*>(arg->data);
+  server->StartShutDown();
+}
+
 bool cmServerBase::Serve(std::string* errorMessage)
 {
 #ifndef NDEBUG
@@ -443,14 +443,13 @@ bool cmServerBase::Serve(std::string* errorMessage)
 
   errorMessage->clear();
 
-  uv_signal_init(&Loop, &this->SIGINTHandler);
-  uv_signal_init(&Loop, &this->SIGHUPHandler);
+  ShutdownSignal.init(Loop, __shutdownThread, this);
 
-  this->SIGINTHandler.data = this;
-  this->SIGHUPHandler.data = this;
+  SIGINTHandler.init(Loop, this);
+  SIGHUPHandler.init(Loop, this);
 
-  uv_signal_start(&this->SIGINTHandler, &on_signal, SIGINT);
-  uv_signal_start(&this->SIGHUPHandler, &on_signal, SIGHUP);
+  SIGINTHandler.start(&on_signal, SIGINT);
+  SIGHUPHandler.start(&on_signal, SIGHUP);
 
   OnServeStart();
 
@@ -473,7 +472,6 @@ bool cmServerBase::Serve(std::string* errorMessage)
     return false;
   }
 
-  ServeThreadRunning = false;
   return true;
 }
 
@@ -487,15 +485,9 @@ void cmServerBase::OnServeStart()
 
 void cmServerBase::StartShutDown()
 {
-  if (!uv_is_closing(
-        reinterpret_cast<const uv_handle_t*>(&this->SIGINTHandler))) {
-    uv_signal_stop(&this->SIGINTHandler);
-  }
-
-  if (!uv_is_closing(
-        reinterpret_cast<const uv_handle_t*>(&this->SIGHUPHandler))) {
-    uv_signal_stop(&this->SIGHUPHandler);
-  }
+  ShutdownSignal.reset();
+  SIGINTHandler.reset();
+  SIGHUPHandler.reset();
 
   {
     cm::unique_lock<cm::shared_mutex> lock(ConnectionsMutex);
@@ -519,20 +511,27 @@ cmServerBase::cmServerBase(cmConnection* connection)
 {
   auto err = uv_loop_init(&Loop);
   (void)err;
+  Loop.data = this;
   assert(err == 0);
 
   AddNewConnection(connection);
 }
 
-cmServerBase::~cmServerBase()
+void cmServerBase::Close()
 {
+  if (Loop.data) {
+    if (ServeThreadRunning) {
+      this->ShutdownSignal.send();
+      uv_thread_join(&ServeThread);
+    }
 
-  if (ServeThreadRunning) {
-    uv_async_send(&this->ShutdownSignal);
-    uv_thread_join(&ServeThread);
+    uv_loop_close(&Loop);
+    Loop.data = nullptr;
   }
-
-  uv_loop_close(&Loop);
+}
+cmServerBase::~cmServerBase()
+{
+  Close();
 }
 
 void cmServerBase::AddNewConnection(cmConnection* ownedConnection)
@@ -562,6 +561,6 @@ void cmServerBase::OnDisconnect(cmConnection* pConnection)
   }
 
   if (Connections.empty()) {
-    StartShutDown();
+    this->ShutdownSignal.send();
   }
 }

Source/cmServer.h

@@ -8,6 +8,8 @@
 #include "cm_thread.hxx"
 #include "cm_uv.h"
 
+#include "cmUVHandlePtr.h"
+
 #include <memory> // IWYU pragma: keep
 #include <string>
 #include <vector>
@@ -58,7 +60,7 @@ public:
 
   virtual bool OnSignal(int signum);
   uv_loop_t* GetLoop();
-
+  void Close();
   void OnDisconnect(cmConnection* pConnection);
 
 protected:
@@ -67,7 +69,7 @@ protected:
 
   bool ServeThreadRunning = false;
   uv_thread_t ServeThread;
-  uv_async_t ShutdownSignal;
+  cm::uv_async_ptr ShutdownSignal;
 #ifndef NDEBUG
 public:
   // When the server starts it will mark down it's current thread ID,
@@ -80,8 +82,8 @@ protected:
 
   uv_loop_t Loop;
 
-  uv_signal_t SIGINTHandler;
-  uv_signal_t SIGHUPHandler;
+  cm::uv_signal_ptr SIGINTHandler;
+  cm::uv_signal_ptr SIGHUPHandler;
 };
 
 class cmServer : public cmServerBase

Source/cmServerConnection.cxx

@@ -5,6 +5,9 @@
 #include "cmConfigure.h"
 #include "cmServer.h"
 #include "cmServerDictionary.h"
+#include "cm_uv.h"
+
+#include <algorithm>
 #ifdef _WIN32
 #include "io.h"
 #else
@@ -18,36 +21,34 @@ cmStdIoConnection::cmStdIoConnection(
 {
 }
 
-void cmStdIoConnection::SetupStream(uv_stream_t*& stream, int file_id)
+cm::uv_stream_ptr cmStdIoConnection::SetupStream(int file_id)
 {
-  assert(stream == nullptr);
   switch (uv_guess_handle(file_id)) {
     case UV_TTY: {
-      auto tty = new uv_tty_t();
-      uv_tty_init(this->Server->GetLoop(), tty, file_id, file_id == 0);
+      cm::uv_tty_ptr tty;
+      tty.init(*this->Server->GetLoop(), file_id, file_id == 0,
+               static_cast<cmEventBasedConnection*>(this));
       uv_tty_set_mode(tty, UV_TTY_MODE_NORMAL);
-      stream = reinterpret_cast<uv_stream_t*>(tty);
-      break;
+      return std::move(tty);
     }
     case UV_FILE:
       if (file_id == 0) {
-        return;
+        return nullptr;
       }
       // Intentional fallthrough; stdin can _not_ be treated as a named
       // pipe, however stdout can be.
       CM_FALLTHROUGH;
     case UV_NAMED_PIPE: {
-      auto pipe = new uv_pipe_t();
-      uv_pipe_init(this->Server->GetLoop(), pipe, 0);
+      cm::uv_pipe_ptr pipe;
+      pipe.init(*this->Server->GetLoop(), 0,
+                static_cast<cmEventBasedConnection*>(this));
       uv_pipe_open(pipe, file_id);
-      stream = reinterpret_cast<uv_stream_t*>(pipe);
-      break;
+      return std::move(pipe);
     }
     default:
       assert(false && "Unable to determine stream type");
-      return;
+      return nullptr;
   }
-  stream->data = static_cast<cmEventBasedConnection*>(this);
 }
 
 void cmStdIoConnection::SetServer(cmServerBase* s)
@@ -57,14 +58,14 @@ void cmStdIoConnection::SetServer(cmServerBase* s)
     return;
   }
 
-  SetupStream(this->ReadStream, 0);
-  SetupStream(this->WriteStream, 1);
+  this->ReadStream = SetupStream(0);
+  this->WriteStream = SetupStream(1);
 }
 
 void shutdown_connection(uv_prepare_t* prepare)
 {
   cmStdIoConnection* connection =
-    reinterpret_cast<cmStdIoConnection*>(prepare->data);
+    static_cast<cmStdIoConnection*>(prepare->data);
 
   if (!uv_is_closing(reinterpret_cast<uv_handle_t*>(prepare))) {
     uv_close(reinterpret_cast<uv_handle_t*>(prepare),
@@ -76,7 +77,7 @@ void shutdown_connection(uv_prepare_t* prepare)
 bool cmStdIoConnection::OnServeStart(std::string* pString)
 {
   Server->OnConnected(this);
-  if (this->ReadStream) {
+  if (this->ReadStream.get()) {
     uv_read_start(this->ReadStream, on_alloc_buffer, on_read);
   } else if (uv_guess_handle(0) == UV_FILE) {
     char buffer[1024];
@@ -94,44 +95,14 @@ bool cmStdIoConnection::OnServeStart(std::string* pString)
   return cmConnection::OnServeStart(pString);
 }
 
-void cmStdIoConnection::ShutdownStream(uv_stream_t*& stream)
-{
-  if (!stream) {
-    return;
-  }
-  switch (stream->type) {
-    case UV_TTY: {
-      assert(!uv_is_closing(reinterpret_cast<uv_handle_t*>(stream)));
-      if (!uv_is_closing(reinterpret_cast<uv_handle_t*>(stream))) {
-        uv_close(reinterpret_cast<uv_handle_t*>(stream),
-                 &on_close_delete<uv_tty_t>);
-      }
-      break;
-    }
-    case UV_FILE:
-    case UV_NAMED_PIPE: {
-      assert(!uv_is_closing(reinterpret_cast<uv_handle_t*>(stream)));
-      if (!uv_is_closing(reinterpret_cast<uv_handle_t*>(stream))) {
-        uv_close(reinterpret_cast<uv_handle_t*>(stream),
-                 &on_close_delete<uv_pipe_t>);
-      }
-      break;
-    }
-    default:
-      assert(false && "Unable to determine stream type");
-  }
-
-  stream = nullptr;
-}
-
 bool cmStdIoConnection::OnConnectionShuttingDown()
 {
-  if (ReadStream) {
+  if (ReadStream.get()) {
     uv_read_stop(ReadStream);
+    ReadStream->data = nullptr;
   }
 
-  ShutdownStream(ReadStream);
-  ShutdownStream(WriteStream);
+  this->ReadStream.reset();
 
   cmEventBasedConnection::OnConnectionShuttingDown();
 

Source/cmServerConnection.h

@@ -8,7 +8,7 @@
 
 #include "cmConnection.h"
 #include "cmPipeConnection.h"
-#include "cm_uv.h"
+#include "cmUVHandlePtr.h"
 
 class cmServerBase;
 
@@ -46,8 +46,8 @@ public:
   bool OnServeStart(std::string* pString) override;
 
 private:
-  void SetupStream(uv_stream_t*& stream, int file_id);
-  void ShutdownStream(uv_stream_t*& stream);
+  cm::uv_stream_ptr SetupStream(int file_id);
+  cm::uv_stream_ptr ReadStream;
 };
 
 /***

Source/cmUVHandlePtr.cxx

@@ -0,0 +1,198 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file Copyright.txt or https://cmake.org/licensing for details.  */
+#define cmUVHandlePtr_cxx
+#include "cmUVHandlePtr.h"
+
+#include <assert.h>
+#include <stdlib.h>
+
+#include "cm_thread.hxx"
+#include "cm_uv.h"
+
+namespace cm {
+
+static void close_delete(uv_handle_t* h)
+{
+  free(h);
+}
+
+template <typename T>
+static void default_delete(T* type_handle)
+{
+  auto handle = reinterpret_cast<uv_handle_t*>(type_handle);
+  if (handle) {
+    assert(!uv_is_closing(handle));
+    if (!uv_is_closing(handle)) {
+      uv_close(handle, &close_delete);
+    }
+  }
+}
+
+/**
+ * Encapsulates delete logic for a given handle type T
+ */
+template <typename T>
+struct uv_handle_deleter
+{
+  void operator()(T* type_handle) const { default_delete(type_handle); }
+};
+
+template <typename T>
+void uv_handle_ptr_base_<T>::allocate(void* data)
+{
+  reset();
+
+  /*
+    We use calloc since we know all these types are c structs
+    and we just want to 0 init them. New would do the same thing;
+    but casting from uv_handle_t to certain other types -- namely
+    uv_timer_t -- triggers a cast_align warning on certain systems.
+  */
+  handle.reset(static_cast<T*>(calloc(1, sizeof(T))), uv_handle_deleter<T>());
+  handle->data = data;
+}
+
+template <typename T>
+void uv_handle_ptr_base_<T>::reset()
+{
+  handle.reset();
+}
+
+template <typename T>
+uv_handle_ptr_base_<T>::operator uv_handle_t*()
+{
+  return reinterpret_cast<uv_handle_t*>(handle.get());
+}
+
+template <typename T>
+T* uv_handle_ptr_base_<T>::operator->() const noexcept
+{
+  return handle.get();
+}
+
+template <typename T>
+T* uv_handle_ptr_base_<T>::get() const
+{
+  return handle.get();
+}
+
+template <typename T>
+uv_handle_ptr_<T>::operator T*() const
+{
+  return this->handle.get();
+}
+
+template <>
+struct uv_handle_deleter<uv_async_t>
+{
+  /***
+  * Wile uv_async_send is itself thread-safe, there are
+  * no strong guarantees that close hasn't already been
+  * called on the handle; and that it might be deleted
+  * as the send call goes through. This mutex guards
+  * against that.
+  *
+  * The shared_ptr here is to allow for copy construction
+  * which is mandated by the standard for Deleter on
+  * shared_ptrs.
+  */
+  std::shared_ptr<cm::mutex> handleMutex;
+
+  uv_handle_deleter()
+    : handleMutex(std::make_shared<cm::mutex>())
+  {
+  }
+
+  void operator()(uv_async_t* handle)
+  {
+    cm::lock_guard<cm::mutex> lock(*handleMutex);
+    default_delete(handle);
+  }
+};
+
+void uv_async_ptr::send()
+{
+  auto deleter = std::get_deleter<uv_handle_deleter<uv_async_t>>(this->handle);
+  assert(deleter);
+
+  cm::lock_guard<cm::mutex> lock(*deleter->handleMutex);
+  if (this->handle) {
+    uv_async_send(*this);
+  }
+}
+
+int uv_async_ptr::init(uv_loop_t& loop, uv_async_cb async_cb, void* data)
+{
+  allocate(data);
+  return uv_async_init(&loop, handle.get(), async_cb);
+}
+
+template <>
+struct uv_handle_deleter<uv_signal_t>
+{
+  void operator()(uv_signal_t* handle) const
+  {
+    if (handle) {
+      uv_signal_stop(handle);
+      default_delete(handle);
+    }
+  }
+};
+
+int uv_signal_ptr::init(uv_loop_t& loop, void* data)
+{
+  allocate(data);
+  return uv_signal_init(&loop, handle.get());
+}
+
+int uv_signal_ptr::start(uv_signal_cb cb, int signum)
+{
+  assert(handle);
+  return uv_signal_start(*this, cb, signum);
+}
+
+void uv_signal_ptr::stop()
+{
+  if (handle) {
+    uv_signal_stop(*this);
+  }
+}
+
+int uv_pipe_ptr::init(uv_loop_t& loop, int ipc, void* data)
+{
+  allocate(data);
+  return uv_pipe_init(&loop, *this, ipc);
+}
+
+uv_pipe_ptr::operator uv_stream_t*() const
+{
+  return reinterpret_cast<uv_stream_t*>(handle.get());
+}
+
+uv_tty_ptr::operator uv_stream_t*() const
+{
+  return reinterpret_cast<uv_stream_t*>(handle.get());
+}
+
+int uv_tty_ptr::init(uv_loop_t& loop, int fd, int readable, void* data)
+{
+  allocate(data);
+  return uv_tty_init(&loop, *this, fd, readable);
+}
+
+template class uv_handle_ptr_base_<uv_handle_t>;
+
+#define UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(NAME)                              \
+  template class uv_handle_ptr_base_<uv_##NAME##_t>;                          \
+  template class uv_handle_ptr_<uv_##NAME##_t>;
+
+UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(async)
+
+UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(signal)
+
+UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(pipe)
+
+UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(stream)
+
+UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(tty)
+}

Source/cmUVHandlePtr.h

@@ -0,0 +1,200 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file Copyright.txt or https://cmake.org/licensing for details.  */
+#pragma once
+#include "cmConfigure.h" // IWYU pragma: keep
+
+#include <algorithm>
+#include <cstddef>
+#include <memory>
+#include <type_traits>
+
+#include "cm_uv.h"
+
+#define CM_PERFECT_FWD_CTOR(Class, FwdTo)                                     \
+  template <typename... Args>                                                 \
+  Class(Args&&... args)                                                       \
+    : FwdTo(std::forward<Args>(args)...)                                      \
+  {                                                                           \
+  }
+
+namespace cm {
+
+/***
+* RAII class to simplify and insure the safe usage of uv_*_t types. This
+* includes making sure resources are properly freed and contains casting
+* operators which allow for passing into relevant uv_* functions.
+*
+*@tparam T actual uv_*_t type represented.
+*/
+template <typename T>
+class uv_handle_ptr_base_
+{
+protected:
+  template <typename _T>
+  friend class uv_handle_ptr_base_;
+
+  /**
+   * This must be a pointer type since the handle can outlive this class.
+   * When uv_close is eventually called on the handle, the memory the
+   * handle inhabits must be valid until the close callback is called
+   * which can be later on in the loop.
+   */
+  std::shared_ptr<T> handle;
+
+  /**
+   * Allocate memory for the type and optionally set it's 'data' pointer.
+   * Protected since this should only be called for an appropriate 'init'
+   * call.
+   *
+   * @param data data pointer to set
+   */
+  void allocate(void* data = nullptr);
+
+public:
+  CM_DISABLE_COPY(uv_handle_ptr_base_)
+  uv_handle_ptr_base_(uv_handle_ptr_base_&&) noexcept;
+  uv_handle_ptr_base_& operator=(uv_handle_ptr_base_&&) noexcept;
+
+  /**
+   * This move constructor allows us to move out of a more specialized
+   * uv type into a less specialized one. The only constraint is that
+   * the right hand side is castable to T.
+   *
+   * This allows you to return uv_handle_ptr or uv_stream_ptr from a function
+   * that initializes something like uv_pipe_ptr or uv_tcp_ptr and interact
+   * and clean up after it without caring about the exact type.
+   */
+  template <typename S, typename = typename std::enable_if<
+                          std::is_rvalue_reference<S&&>::value>::type>
+  uv_handle_ptr_base_(S&& rhs)
+  {
+    // This will force a compiler error if rhs doesn't have a casting
+    // operator to get T*
+    this->handle = std::shared_ptr<T>(rhs.handle, rhs);
+    rhs.handle.reset();
+  }
+
+  // Dtor and ctor need to be inline defined like this for default ctors and
+  // dtors to work.
+  uv_handle_ptr_base_() {}
+  uv_handle_ptr_base_(std::nullptr_t) {}
+  ~uv_handle_ptr_base_() { reset(); }
+
+  /**
+   * Properly close the handle if needed and sets the inner handle to nullptr
+   */
+  void reset();
+
+  /**
+   * Allow less verbose calling of uv_handle_* functions
+   * @return reinterpreted handle
+   */
+  operator uv_handle_t*();
+
+  T* get() const;
+  T* operator->() const noexcept;
+};
+
+template <typename T>
+inline uv_handle_ptr_base_<T>::uv_handle_ptr_base_(
+  uv_handle_ptr_base_<T>&&) noexcept = default;
+template <typename T>
+inline uv_handle_ptr_base_<T>& uv_handle_ptr_base_<T>::operator=(
+  uv_handle_ptr_base_<T>&&) noexcept = default;
+
+/**
+ * While uv_handle_ptr_base_ only exposes uv_handle_t*, this exposes uv_T_t*
+ * too. It is broken out like this so we can reuse most of the code for the
+ * uv_handle_ptr class.
+ */
+template <typename T>
+class uv_handle_ptr_ : public uv_handle_ptr_base_<T>
+{
+  template <typename _T>
+  friend class uv_handle_ptr_;
+
+public:
+  CM_PERFECT_FWD_CTOR(uv_handle_ptr_, uv_handle_ptr_base_<T>);
+
+  /***
+   * Allow less verbose calling of uv_<T> functions
+   * @return reinterpreted handle
+   */
+  operator T*() const;
+};
+
+/***
+ * This specialization is required to avoid duplicate 'operator uv_handle_t*()'
+ * declarations
+ */
+template <>
+class uv_handle_ptr_<uv_handle_t> : public uv_handle_ptr_base_<uv_handle_t>
+{
+public:
+  CM_PERFECT_FWD_CTOR(uv_handle_ptr_, uv_handle_ptr_base_<uv_handle_t>);
+};
+
+class uv_async_ptr : public uv_handle_ptr_<uv_async_t>
+{
+public:
+  CM_PERFECT_FWD_CTOR(uv_async_ptr, uv_handle_ptr_<uv_async_t>);
+
+  int init(uv_loop_t& loop, uv_async_cb async_cb, void* data = nullptr);
+
+  void send();
+};
+
+struct uv_signal_ptr : public uv_handle_ptr_<uv_signal_t>
+{
+  CM_PERFECT_FWD_CTOR(uv_signal_ptr, uv_handle_ptr_<uv_signal_t>);
+
+  int init(uv_loop_t& loop, void* data = nullptr);
+
+  int start(uv_signal_cb cb, int signum);
+
+  void stop();
+};
+
+struct uv_pipe_ptr : public uv_handle_ptr_<uv_pipe_t>
+{
+  CM_PERFECT_FWD_CTOR(uv_pipe_ptr, uv_handle_ptr_<uv_pipe_t>);
+
+  operator uv_stream_t*() const;
+
+  int init(uv_loop_t& loop, int ipc, void* data = nullptr);
+};
+
+struct uv_tty_ptr : public uv_handle_ptr_<uv_tty_t>
+{
+  CM_PERFECT_FWD_CTOR(uv_tty_ptr, uv_handle_ptr_<uv_tty_t>);
+
+  operator uv_stream_t*() const;
+
+  int init(uv_loop_t& loop, int fd, int readable, void* data = nullptr);
+};
+
+typedef uv_handle_ptr_<uv_stream_t> uv_stream_ptr;
+typedef uv_handle_ptr_<uv_handle_t> uv_handle_ptr;
+
+#ifndef cmUVHandlePtr_cxx
+
+extern template class uv_handle_ptr_base_<uv_handle_t>;
+
+#define UV_HANDLE_PTR_INSTANTIATE_EXTERN(NAME)                                \
+  extern template class uv_handle_ptr_base_<uv_##NAME##_t>;                   \
+  extern template class uv_handle_ptr_<uv_##NAME##_t>;
+
+UV_HANDLE_PTR_INSTANTIATE_EXTERN(async)
+
+UV_HANDLE_PTR_INSTANTIATE_EXTERN(signal)
+
+UV_HANDLE_PTR_INSTANTIATE_EXTERN(pipe)
+
+UV_HANDLE_PTR_INSTANTIATE_EXTERN(stream)
+
+UV_HANDLE_PTR_INSTANTIATE_EXTERN(tty)
+
+#undef UV_HANDLE_PTR_INSTANTIATE_EXTERN
+
+#endif
+}

Source/cm_thread.hxx

@@ -9,6 +9,7 @@
 namespace cm {
 class mutex
 {
+  CM_DISABLE_COPY(mutex)
   uv_mutex_t _M_;
 
 public:
@@ -24,6 +25,7 @@ template <typename T>
 class lock_guard
 {
   T& _mutex;
+  CM_DISABLE_COPY(lock_guard)
 
 public:
   lock_guard(T& m)
@@ -37,6 +39,7 @@ public:
 class shared_mutex
 {
   uv_rwlock_t _M_;
+  CM_DISABLE_COPY(shared_mutex)
 
 public:
   shared_mutex() { uv_rwlock_init(&_M_); }
@@ -55,6 +58,7 @@ template <typename T>
 class shared_lock
 {
   T& _mutex;
+  CM_DISABLE_COPY(shared_lock)
 
 public:
   shared_lock(T& m)
@@ -68,6 +72,8 @@ public:
 template <typename T>
 class unique_lock : public lock_guard<T>
 {
+  CM_DISABLE_COPY(unique_lock)
+
 public:
   unique_lock(T& m)
     : lock_guard<T>(m)

Tests/CMakeLists.txt

@@ -149,9 +149,7 @@ if(BUILD_TESTING)
   if(NOT CMake_TEST_EXTERNAL_CMAKE)
     add_subdirectory(CMakeLib)
 
-    if(CMake_TEST_SERVER_MODE)
-      add_subdirectory(CMakeServerLib)
-    endif()
+    add_subdirectory(CMakeServerLib)
   endif()
   add_subdirectory(CMakeOnly)
   add_subdirectory(RunCMake)

Tests/CMakeServerLib/CMakeLists.txt

@@ -6,12 +6,16 @@ include_directories(
 
 set(CMakeServerLib_TESTS
   testServerBuffering
+  testUVRAII
   )
 
 create_test_sourcelist(CMakeLib_TEST_SRCS CMakeServerLibTests.cxx ${CMakeServerLib_TESTS})
 add_executable(CMakeServerLibTests ${CMakeLib_TEST_SRCS})
 target_link_libraries(CMakeServerLibTests CMakeLib CMakeServerLib)
 
+SET_PROPERTY(TARGET CMakeServerLibTests PROPERTY C_CLANG_TIDY "")
+SET_PROPERTY(TARGET CMakeServerLibTests PROPERTY CXX_CLANG_TIDY "")
+
 foreach(test ${CMakeServerLib_TESTS})
   add_test(CMakeServerLib.${test} CMakeServerLibTests ${test} ${${test}_ARGS})
 endforeach()

Tests/CMakeServerLib/testServerBuffering.cpp

@@ -1,7 +1,6 @@
 #include "cmConnection.h"
 #include "cmServerConnection.h"
 #include <iostream>
-#include <stddef.h>
 #include <string>
 #include <vector>
 
@@ -51,8 +50,8 @@ int testServerBuffering(int, char** const)
     std::unique_ptr<cmConnectionBufferStrategy>(new cmServerBufferStrategy);
   std::vector<std::string> response;
   std::string rawBuffer;
-  for (size_t i = 0; i < fullMessage.size(); i++) {
-    rawBuffer += fullMessage[i];
+  for (auto& messageChar : fullMessage) {
+    rawBuffer += messageChar;
     std::string packet = bufferingStrategy->BufferMessage(rawBuffer);
     do {
       if (!packet.empty() && packet != "\r\n") {

Tests/CMakeServerLib/testUVRAII.cxx

@@ -0,0 +1,179 @@
+#include "cmUVHandlePtr.h"
+
+#include <algorithm>
+#include <chrono>
+#include <iostream>
+#include <thread>
+
+#include "cm_uv.h"
+
+static void signal_reset_fn(uv_async_t* handle)
+{
+  auto ptr = static_cast<cm::uv_async_ptr*>(handle->data);
+  ptr->reset();
+}
+
+// A common pattern is to use an async signal to shutdown the server.
+static bool testAsyncShutdown()
+{
+  uv_loop_t Loop;
+  auto err = uv_loop_init(&Loop);
+  if (err != 0) {
+    std::cerr << "Could not init loop" << std::endl;
+    return false;
+  }
+
+  {
+    cm::uv_async_ptr signal;
+    signal.init(Loop, &signal_reset_fn, &signal);
+
+    std::thread([&] {
+      std::this_thread::sleep_for(std::chrono::seconds(2));
+      signal.send();
+    }).detach();
+
+    if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) {
+      std::cerr << "Unclean exit state in testAsyncDtor" << std::endl;
+      return false;
+    }
+
+    if (signal.get()) {
+      std::cerr << "Loop exited with signal not being cleaned up" << std::endl;
+      return false;
+    }
+  }
+
+  uv_loop_close(&Loop);
+
+  return true;
+}
+
+static void signal_fn(uv_async_t*)
+{
+}
+
+// Async dtor is sort of a pain; since it locks a mutex we must be sure its
+// dtor always calls reset otherwise the mutex is deleted then locked.
+static bool testAsyncDtor()
+{
+  uv_loop_t Loop;
+  auto err = uv_loop_init(&Loop);
+  if (err != 0) {
+    std::cerr << "Could not init loop" << std::endl;
+    return false;
+  }
+
+  {
+    cm::uv_async_ptr signal;
+    signal.init(Loop, signal_fn);
+  }
+
+  if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) {
+    std::cerr << "Unclean exit state in testAsyncDtor" << std::endl;
+    return false;
+  }
+
+  uv_loop_close(&Loop);
+
+  return true;
+}
+
+// Async needs a relatively stateful deleter; make sure that is properly
+// accounted for and doesn't try to hold on to invalid state when it is
+// moved
+static bool testAsyncMove()
+{
+  uv_loop_t Loop;
+  auto err = uv_loop_init(&Loop);
+  if (err != 0) {
+    std::cerr << "Could not init loop" << std::endl;
+    return false;
+  }
+
+  {
+    cm::uv_async_ptr signal;
+    {
+      cm::uv_async_ptr signalTmp;
+      signalTmp.init(Loop, signal_fn);
+      signal = std::move(signalTmp);
+    }
+  }
+
+  if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) {
+    std::cerr << "Unclean exit state in testAsyncDtor" << std::endl;
+    return false;
+  }
+
+  uv_loop_close(&Loop);
+  return true;
+}
+
+// When a type is castable to another uv type (pipe -> stream) here,
+// and the deleter is convertible as well, we should allow moves from
+// one type to the other.
+static bool testCrossAssignment()
+{
+  uv_loop_t Loop;
+  auto err = uv_loop_init(&Loop);
+  if (err != 0) {
+    std::cerr << "Could not init loop" << std::endl;
+    return false;
+  }
+
+  {
+    cm::uv_pipe_ptr pipe;
+    pipe.init(Loop, 0);
+
+    cm::uv_stream_ptr stream = std::move(pipe);
+    if (pipe.get()) {
+      std::cerr << "Move should be sure to invalidate the previous ptr"
+                << std::endl;
+      return false;
+    }
+    cm::uv_handle_ptr handle = std::move(stream);
+    if (stream.get()) {
+      std::cerr << "Move should be sure to invalidate the previous ptr"
+                << std::endl;
+      return false;
+    }
+  }
+
+  if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) {
+    std::cerr << "Unclean exit state in testCrossAssignment" << std::endl;
+    return false;
+  }
+
+  uv_loop_close(&Loop);
+  return true;
+}
+
+// This test can't fail at run time; but this makes sure we have all our move
+// ctors created correctly.
+static bool testAllMoves()
+{
+  using namespace cm;
+  struct allTypes
+  {
+    uv_stream_ptr _7;
+    uv_tty_ptr _9;
+    uv_pipe_ptr _12;
+    uv_async_ptr _13;
+    uv_signal_ptr _14;
+    uv_handle_ptr _15;
+  };
+
+  allTypes a;
+  allTypes b(std::move(a));
+  allTypes c = std::move(b);
+  return true;
+};
+
+int testUVRAII(int, char** const)
+{
+  if ((testAsyncShutdown() &&
+       testAsyncDtor() & testAsyncMove() & testCrossAssignment() &
+         testAllMoves()) == 0) {
+    return -1;
+  }
+  return 0;
+}